Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI refactoring #573

Merged
merged 28 commits into from
Feb 11, 2024
Merged

CI refactoring #573

merged 28 commits into from
Feb 11, 2024

Conversation

bennibbelink
Copy link
Contributor

This PR should be considered in conjunction with cyclus PR #1637.

The main changes being made are converting CI workflows to use docker/build-push-action to build and test cycamore (and downstream cymetric) using the Dockerfiles we maintain.

Some notes:

  • the Dockerfile now takes a build argument to specify the tag of the cyclus image to build from
  • we use our ghcr registry as a cache backend, such that cached layers are located at :ci-layer-cache
  • we also use ghcr to store images produced from workflow runs, located at :ci-image-cache. This allows us to build downstream projects from a unique cycamore image (which is specified by digest in the workflow)
  • I added a workflow to publish images on release and tag them as :stable and :<version number>. It only builds against cyclus:stable. The idea being that we would publish a cyclus release as stable, then have a corresponding cycamore release also tagged as stable
  • the build_test workflow includes a matrix variable for the cyclus tag to build cycamore against (stable and latest)
  • Cycamore build failures will still pass CI, a bot will comment the build statuses of each respective cycamore build (I wanted to just make it a single comment but struggled to find a way to do it generically with the testing matrix)

Copy link

Build statuses using cyclus_20.04_apt:stable

  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️

Copy link

Build statuses using cyclus_20.04_conda:stable

  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️

Copy link

Build statuses using cyclus_22.04_apt:stable

  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️

Copy link

Build statuses using cyclus_22.04_conda:stable

  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️

Copy link

github-actions bot commented Jan 29, 2024

Build statuses using cyclus_20.04_apt:latest

  • Cycamore: Success
  • Cymetric: Success

Copy link

github-actions bot commented Jan 29, 2024

Build statuses using cyclus_20.04_conda:latest

  • Cycamore: Success
  • Cymetric: Success

Copy link

github-actions bot commented Jan 29, 2024

Build statuses using cyclus_22.04_apt:latest

  • Cycamore: Success
  • Cymetric: Success

Copy link

github-actions bot commented Jan 29, 2024

Build statuses using cyclus_22.04_conda:latest

  • Cycamore: Success
  • Cymetric: Success

@bennibbelink bennibbelink marked this pull request as ready for review February 9, 2024 04:26
Copy link
Member

@abachma2 abachma2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes @bennibbelink. Just to make sure I understand what the new CI workflow would be:

  • There is one workflow for building Cycamore (using a Docker image of Cyclus) and running the unit tests (build_test.yml). This one runs on any PR or push into cyclus:main.
  • There is one workflow for making sure the CHANGELOG is updated (changelog_test.yml). This one runs on any PR or push to cyclus:main.
  • There is one workflow for publishing the latest Cycamore Docker image (publish_latest.yml). This runs on a PR or push into cyclus:main. Is there any reason for this one to checkout and text Cymetric?
  • There is one workflow for publishing a Docker image of a Cycamore (publish_release.yml). This one runs on a push to cyclus:main
    Can you provide some explanation for the difference between the publish_latest.yml and publish_release.yml? I see the addition of building and testing cymetric for latest, and the tags are a bit different, but I'm not seeing much else.

@bennibbelink
Copy link
Contributor Author

@abachma2 your descriptions are mostly correct, here are some corrections in bold:

  • There is one workflow for building Cycamore (using a Docker image of Cyclus) and running the unit tests (build_test.yml). This one runs on any PR or push into any branch of cycamore. This also checks out and builds Cymetric, and comments the Cymetric build status in the PR conversation. This is to ensure that we are aware of whether changes to Cycamore will change Cymetric (without blocking the PR).
  • There is one workflow for making sure the CHANGELOG is updated (changelog_test.yml). This one runs on any PR (not on any pushes).
  • There is one workflow for publishing the latest Cycamore Docker image (publish_latest.yml). This runs on a PR that modifies publish_latest.yml or push into cyclus:main. It checks out and builds/publishes cymetric so that the cymetric:latest image in GHCR is always up to date with the latest cycamore library. This will occur in cyclus too, see publish_latest.yml in #1637 (pending review)
  • There is one workflow for publishing a Docker image of a Cycamore (publish_release.yml). This one runs on a push to cyclus:main when a tag is present. This is intended to push images of cycamore tagged stable and <version number> when we issue a release. The workflow may change pending review from @gonuke, we might need to use different triggers.

Can you provide some explanation for the difference between the publish_latest.yml and publish_release.yml? I see the addition of building and testing cymetric for latest, and the tags are a bit different, but I'm not seeing much else.

I think I answered this in the bullets above but please ask more questions if you have them!!! This is a big PR (collectively across three repos) and I want to make sure there are plenty of eyes on these changes. The stable, latest, and <version number> tags should be clear now. As for the ci-image-cache and ci-layer-cache tags it may be helpful to review this comment and diagram from the conversation in #1637.

@abachma2
Copy link
Member

Those explanations make a lot of sense, and match what I see in the code (I'm not great with reading CI code, but with the explanations it makes sense). I will go ahead and approve this PR. Let me know when it can/should get merged. I'm not sure how this will effect things if all three aren't merged at the same time.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comments to cymetric PR as well as tagging some cleanup needs on coordinated PRs

.github/workflows/build_test.yml Outdated Show resolved Hide resolved
.github/workflows/publish_latest.yml Show resolved Hide resolved
.github/workflows/publish_latest.yml Outdated Show resolved Hide resolved
.github/workflows/publish_release.yml Outdated Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Feb 11, 2024

Ready to merge after tag cleanup following cymetric merge

@bennibbelink
Copy link
Contributor Author

Cleaned up steps checking out cymetric. Ready to merge

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bennibbelink

@gonuke gonuke merged commit 2fc4d49 into main Feb 11, 2024
21 checks passed
@bennibbelink bennibbelink deleted the tag-build-arg branch February 16, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants