-
Notifications
You must be signed in to change notification settings - Fork 520
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 pipeline tooling for signing tool #654
Conversation
e33c09e
to
4bec950
Compare
Renamed the wrapper script with skewers to be more uniform with the rest of the tooling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of things that'll keep the wrapper from running, let's get that fixed and get rolling
4bec950
to
c29c6ab
Compare
Address @etungsten and @jahkeup 's comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer that we didn't bundle the builder environment work in the base stage, can we move that to a different stage? I left an example of what I mean and one way that this could be done in the review comments.
c29c6ab
to
78d9bcc
Compare
Updated the Dockerfile per @jahkeup and fix the type in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to expand the indirection handling in the wrapper to make it clearer at higher level "call sites" that we're passing a variable name down into the build. This also gives us a chance to add some quick checks too that can be caught before entering the signing code.
78d9bcc
to
d4632f3
Compare
Address @jahkeup 's comments! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Let's get this built into the container and tested in the pipeline!
@zmrow Github thinks you need to rebase/merge |
d4632f3
to
d53c7d7
Compare
Issue #, if available:
Related to #592 and #608
Description of changes:
Testing done:
Once the container finishes building I can test the tooling.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.