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

Add github actions workflow #4

Merged
merged 8 commits into from
Sep 27, 2024
Merged

Conversation

sarahchen6
Copy link
Contributor

@sarahchen6 sarahchen6 commented Sep 13, 2024

This PR creates a GitHub Actions workflow that builds the catadog Docker image in CI for both x86-64 and aarch64-linux architectures. The workflow builds specifically for ruby 3.4, and the pushed image is located at ghcr.io/datadog/catadog:latest. The image currently has no tag, and it is only pushed when manually dispatched. NOTE that though the workflow is technically running the test suite, as of this PR there are no catadog tests in this test suite.

After merging, there is also suspected flakiness in Github Actions: 401 unauthorization error when pulling image from docker.io.

@sarahchen6 sarahchen6 force-pushed the sarahchen6/add-github-workflow branch 2 times, most recently from 8c6162c to afe7353 Compare September 18, 2024 20:18
@sarahchen6 sarahchen6 force-pushed the sarahchen6/add-github-workflow branch from afe7353 to c12112e Compare September 18, 2024 20:23
@sarahchen6 sarahchen6 force-pushed the sarahchen6/add-github-workflow branch from c12112e to 1bf981b Compare September 18, 2024 21:02
@sarahchen6 sarahchen6 force-pushed the sarahchen6/add-github-workflow branch from 1418fa1 to 36a3b23 Compare September 18, 2024 21:26
@sarahchen6 sarahchen6 marked this pull request as ready for review September 19, 2024 20:49
@sarahchen6 sarahchen6 force-pushed the sarahchen6/add-github-workflow branch 2 times, most recently from 710afbd to e2e713c Compare September 20, 2024 18:07
TonyCTHsu
TonyCTHsu previously approved these changes Sep 20, 2024
@sarahchen6 sarahchen6 marked this pull request as draft September 20, 2024 19:47
@TonyCTHsu TonyCTHsu dismissed their stale review September 20, 2024 20:31

still WIP

@sarahchen6 sarahchen6 force-pushed the sarahchen6/add-github-workflow branch from e0ee625 to b21b304 Compare September 23, 2024 14:29
@sarahchen6 sarahchen6 marked this pull request as ready for review September 23, 2024 16:58
- name: Build multi-arch image (x86-64, aarch64)
if: ${{ inputs.push }}
run: |
docker buildx build . --builder=container --cache-from=type=registry,ref=ghcr.io/datadog/catadog --output=type=image,push=true --build-arg BUILDKIT_INLINE_CACHE=1 --platform linux/x86_64,linux/aarch64 -f ./Dockerfile --tag ghcr.io/datadog/catadog
Copy link
Contributor Author

@sarahchen6 sarahchen6 Sep 23, 2024

Choose a reason for hiding this comment

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

The image is currently being pushed as ghcr.io/datadog/catadog with no tag. One suggestion was to add the tag based on the catadog version, but I'm not sure how to get the version number if there are no official releases. Another suggestion was to pair the image with the github.run_id. However, I thought that may be too unique of an identifier, as each workflow run would have a different run_id. The workflow that this is based on tags the image based on the ruby / jruby version. Since this workflow only tests one ruby version (3.4), I did not see the value of adding the 3.4 tag.

Should there still be a tag though? and if so, what could that tag be?

Copy link

Choose a reason for hiding this comment

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

tag based on the catadog version

Yeah this was my suggestion. I think it would mean storing the version in a plain-text file and sourcing the file to set catadog's version and the image's version. Alternatively, if there was some kind of gem or bundle command that could parse out the version from the .gemspec, that could work too. I'm afraid I don't know what the latter would look off the top of my head.

I don't recommend run ID, mostly because its not meaningfully linked to a particular state of the Ruby gem. At least, it would require some digging through build logs to figure out what version was being built at the time... not a fun experience.

Copy link
Member

@lloeki lloeki Oct 1, 2024

Choose a reason for hiding this comment

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

I don't recommend run ID, mostly because its not meaningfully linked to a particular state of the Ruby gem

run_id is essential for auditability and reproducibility: two runs of the same commit may produce different results because of environment changes.

Note that run_id is used only to use the proper image across, say, a workflow's jobs. Without run_id one runs into concurrency issues: imagine two runs are triggered for the same commit and the only identifier is the commid id; since we don't control the scheduling of jobs, if the sole identifier is the commit id there is an opportunity for a run to end up picking the other run's output.

This process is entirely different and decoupled from a release process, where there would be a version, a tag, and a single set of artifacts, which, once published, should be immutable. Nonetheless, the release process can reuse the run artifacts and promote them to be tagged with a simple version instead.

I think it would mean storing the version in a plain-text file and sourcing the file to set catadog's version and the image's version

The source of truth is the version in the gemspec, which gets tagged on the repository upon release via rake release / Trusted Publishing.

Copy link

@delner delner left a comment

Choose a reason for hiding this comment

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

Overall looks good, as we discussed. I like that we simplified this a bit, excluded the matrix (although cool, seemed overkill for this application.)

Adding a version tag I think is a good idea, but we could do that separately if this PR itself adds self-contained value. In which case, I think this is OK to merge.

@sarahchen6 sarahchen6 merged commit e5655db into main Sep 27, 2024
2 checks passed
@sarahchen6 sarahchen6 deleted the sarahchen6/add-github-workflow branch September 27, 2024 21:04
@sarahchen6 sarahchen6 restored the sarahchen6/add-github-workflow branch September 27, 2024 21:21
@sarahchen6 sarahchen6 deleted the sarahchen6/add-github-workflow branch September 30, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants