-
Notifications
You must be signed in to change notification settings - Fork 528
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
Added env vars and optional layers to mutate cmd #1199
Conversation
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.
We'll need to regen docs again, but overall this looks great. Much simpler than the whole build
command, and pretty much just as powerful, especially with the <(tar ...)
trick documented in recipes.md.
Thanks for doing this!
Glad this fits better.
I've fixed the nits. Let me know what else is needed to gets this upstreamed
Thanks
…On Tue, Dec 7, 2021, 17:28 Jason Hall ***@***.***> wrote:
***@***.**** commented on this pull request.
We'll need to regen docs again, but overall this looks great. Much simpler
than the whole build command, and pretty much just as powerful,
especially with the <(tar ...) trick documented in recipes.md.
Thanks for doing this!
------------------------------
In cmd/crane/cmd/mutate.go
<#1199 (comment)>
:
> + // set envvars if specified - split to a function as it is more
+ // complex then labels because in the config the key value pairs
+ // are just strings with a = in them
Thanks for this context, and I agree, but I'm not sure the comment is
useful outside the context of this code review.
⬇️ Suggested change
- // set envvars if specified - split to a function as it is more
- // complex then labels because in the config the key value pairs
- // are just strings with a = in them
+ // set envvars if specified.
------------------------------
In cmd/crane/cmd/mutate.go
<#1199 (comment)>
:
> + err = setEnvVars(&cfg.Config, envVars)
+ if err != nil {
+ return err
+ }
nit:
⬇️ Suggested change
- err = setEnvVars(&cfg.Config, envVars)
- if err != nil {
- return err
- }
+ if err := setEnvVars(&cfg.Config, envVars); err != nil {
+ return err
+ }
(This makes it clearer that the err isn't going to escape this scope)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1199 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHXLUAVVJQSXGUNISBHCW53UPYR2LANCNFSM5JRDKTIQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Need to run |
Codecov Report
@@ Coverage Diff @@
## main #1199 +/- ##
=======================================
Coverage 74.76% 74.76%
=======================================
Files 111 111
Lines 8008 8008
=======================================
Hits 5987 5987
Misses 1441 1441
Partials 580 580 Continue to review full report at Codecov.
|
uppercase keys in case of Windows Image
No description provided.