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:DOCS] Add github-action workflow to build/push multi-arch #10107

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

cevich
Copy link
Member

@cevich cevich commented Apr 21, 2021

Fixes #4773

This borrows very heavily from the work done for buildah by @barthy1 -
Yulia Gaponenko [email protected]. Some changes to code and
comments made for clarity and specificity.

Ref:

@cevich cevich marked this pull request as draft April 21, 2021 14:05
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2021
@cevich cevich force-pushed the multi_arch_images branch 8 times, most recently from af69631 to 30a1b93 Compare April 22, 2021 13:09
@cevich cevich marked this pull request as ready for review April 22, 2021 13:11
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2021
@cevich
Copy link
Member Author

cevich commented Apr 22, 2021

@barthy1 @rhatdan @TomSweeneyRedHat PTAL this is ready. I've tested the image-building part up to the point of pushing. However due to github action security, the username/password secrets are not accessible from a PR. So I'll keep an eye on this after it merges, to make sure the image push happens as intended.

@cevich
Copy link
Member Author

cevich commented Apr 22, 2021

Q: Should I migrate c/skopeo to use github actions (similar to this and c/buildah) instead of travis?

Assuming so, rather than duplicate substantially the same operations x3 repos., I'd like to make a single "composite-action". This can then be reused instead of being duplicated. It's a bit more work up-front, but will ease the maintenance burden long-term.

@rhatdan
Copy link
Member

rhatdan commented Apr 22, 2021

/approve
LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2021
@rhatdan
Copy link
Member

rhatdan commented Apr 22, 2021

@edsantiago @TomSweeneyRedHat PTAL

@cevich
Copy link
Member Author

cevich commented Apr 22, 2021

@TomSweeneyRedHat I can take care of disabling the auto-builds on quay. Do you have anything related running under cron that needs to be turned off so we don't get clashes?

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

I'm not qualified to review this, I know nothing about github actions, but I've done the best I can and have a few suggestions.

.github/workflows/multi-arch-build.yaml Outdated Show resolved Hide resolved
.github/workflows/multi-arch-build.yaml Outdated Show resolved Hide resolved

# Hack to set $LABELS env. var. in _future_ steps.
# https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-environment-variable
cat << EOF | tee $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

cat-without-args tends to raise eyebrows. Is the intention here to display those values to stdout, perhaps for an output log?

Also, I had to look up $GITHUB_ENV, so I'm not entirely sure what it is or how it works, but shouldn't this be tee -a to prevent an existing file from getting clobbered?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should raise eyebrows...(unfortunately) like many other aspects, this construct is idiomatic to github-actions ☹️ That's why I dropped the docs URL comment, and I'm glad that found some use already 😃

The file never exists, the step is suppose to create it to that triggers parsing of the contents. The echo '::set-output name=foo::bar' idiom only works for a simple single-line value (because reasons). There's no other way to pass a multi-line value to subsequent actions (i.e. docker/build-push-action@v2). This is the best M$ GitHub could come up with 😖

In buildah, the matching step uses a simple here-document redirect to a file. As you guessed, that means no record of the value shows in the logs. This is/was hiding an actual bug. I only discovered it by manually doing a skopeo inspect on one of the images 😠 So I changed it here to use tee and make it show up in the logs to prevent future headaches.

TBH I'm not sure how to make this section any clearer, it's kind of broken by the workflow "language" design 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I've tried twice to understand that, and failed. But it's not important, I don't really need to understand it. My point is: all the github examples use >> (append). In the line above, you use tee (clobber), not tee -a (append). This is inconsistent with the examples documented in github.com itself, and could potentially overwrite something important, so I wanted to make sure that your use is deliberate.

.github/workflows/multi-arch-build.yaml Outdated Show resolved Hide resolved
@cevich cevich force-pushed the multi_arch_images branch from 30a1b93 to 384b836 Compare April 22, 2021 19:28
@cevich
Copy link
Member Author

cevich commented Apr 22, 2021

Thanks for taking a look @edsantiago, I implemented all your suggestions save for the stupid here-document hack 🤕

@cevich cevich force-pushed the multi_arch_images branch from 384b836 to f7dd83f Compare April 22, 2021 19:35
@cevich
Copy link
Member Author

cevich commented Apr 22, 2021

force-push, updated URL to be even more specific re: here-document hack.

This borrows very heavily from the work done for buildah by @barthy1 -
Yulia Gaponenko <[email protected]>.  Some changes to code and
comments made for clarity and specificity.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the multi_arch_images branch from f7dd83f to 3c5bb7b Compare April 22, 2021 20:21
@cevich
Copy link
Member Author

cevich commented Apr 22, 2021

force-pushed - found & fixed a comment typo.

@edsantiago
Copy link
Member

/lgtm
/hold

Please review my concern re: clobbering the $GITHUB_ENV file. If clobbering is ok, feel free to release the hold.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2021
@cevich
Copy link
Member Author

cevich commented Apr 23, 2021

Please review my concern re: clobbering the $GITHUB_ENV file. If clobbering is ok, feel free to release the hold.

Yes, clobbering is intentional (and required). I was about to go and beef up the commenting on this just now...but this usage really is idiomatic to github actions workflow. Everybody would need to add comments when using this method to set multi-line env. vars. Have I mentioned how stupid I think this design is?

@cevich
Copy link
Member Author

cevich commented Apr 23, 2021

Dug up more history and info. First off, this workflow "feature" is a response to a security vulnerability. Second, I confirmed $GITHUB_ENV always points at a per-step temporary file. The file is intended for this specific purpose. Though they don't seem to mention or deal with the case of a value itself containing the \n<FOO>=<BAR>\n string (pattern) 😖 So really, to be safe this file must only/always ever be used to set one value. Otherwise it would be impossible to spot this nested-token design defect. Have I mentioned how stupid I think this is?

@cevich
Copy link
Member Author

cevich commented Apr 23, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2021
@openshift-merge-robot openshift-merge-robot merged commit ba60821 into containers:master Apr 23, 2021
@cevich
Copy link
Member Author

cevich commented Apr 23, 2021

@cevich cevich deleted the multi_arch_images branch June 30, 2021 18:08
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide arm64 multiarch images on quay.io
5 participants