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

Switch should_sign to an output and add signing data #40

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

LaurentGoderre
Copy link
Member

@LaurentGoderre LaurentGoderre commented Sep 13, 2024

This serves multiple purpose. Using output instead of environment allows using the data in subsequence jobs as well as steps making it more flexible. The PR also gets the data necessary for signing from the builds.json so the file isn't needed in subsequent jobs.

Copy link
Collaborator

@whalelines whalelines left a comment

Choose a reason for hiding this comment

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

LGTM

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@LaurentGoderre

This comment was marked as outdated.

@LaurentGoderre LaurentGoderre force-pushed the should-sign-output branch 3 times, most recently from 9805817 to 232c17c Compare September 19, 2024 20:43
@LaurentGoderre
Copy link
Member Author

@tianon this is ready to go

@yosifkit
Copy link
Member

Is there a way to add more to this so that it is all the workflow logic changes (jobs, steps, outputs, artifacts?, etc) but still without the signing changes? Right now, this PR feels like a producer without a consumer or an unused function; it removes the necessary permission while adding variables and an output that aren't used. (one output is used as it replaces a previous env var). And I'd rather that we keep a complete workflow where all things are used.

So, I think this PR should also have the Sign steps in their own job (one job with multiple steps or multiple jobs 🤷) so that we can have a complete change that exercises the GITHUB_ENV as well as the new GITHUB_OUTPUT. And, I guess, the changes to have the Sign and Push jobs consume the image as an artifact. Maybe that's somewhere between this and #41?

@LaurentGoderre LaurentGoderre force-pushed the should-sign-output branch 2 times, most recently from 6493476 to fe95afc Compare September 23, 2024 14:08
@whalelines
Copy link
Collaborator

All the changes are in place and shown to be working in the test repo, right? The idea was to provide multiple smaller PRs so things are easier to review. If you want to see them all together you can look in the testing repo.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@tianon tianon merged commit 4fdf2eb into docker-library:subset Oct 10, 2024
@LaurentGoderre LaurentGoderre deleted the should-sign-output branch October 11, 2024 14:23
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