Refactor command line arguments and the executor #306
Conversation
In this refactor I: 1. Created KanikoOptions to make it easier to pass around arguments passed in through the command line 2. Reorganized executor.go by putting the logic for pushing the image in a new file push.go 3. Made some error messages clearer 4. Fixed a mistake in the README for pushing to AWS 5. Marked the --bucket flag as hidden since we want people to use --context instead, and marked an aws flag as hidden which is set in a vendored directorya
|
/help |
|
@dlorenc: Please ensure the request meets the requirements listed here. If this request no longer meets these requirements, the label can be removed In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @tstromberg |
| @@ -240,7 +240,7 @@ To configure credentials, you will need to do the following: | |||
| - name: aws-secret | |||
| mountPath: /root/.aws/ | |||
| - name: docker-config | |||
| mountPath: /root/.docker/ | |||
| mountPath: /kaniko/.docker/ | |||
There was a problem hiding this comment.
Is this intentional? It's not mentioned in the PR description.
There was a problem hiding this comment.
Yah, this is the error in the README I mentioned in the 4th point :)
There was a problem hiding this comment.
OK, I missed that. I thought this was just a YAML file based on the diff context but didn't read the filename =)
cmd/executor/cmd/root.go
Outdated
| RootCmd.PersistentFlags().BoolVarP(&reproducible, "reproducible", "", false, "Strip timestamps out of the image to make it reproducible") | ||
| RootCmd.PersistentFlags().StringVarP(&target, "target", "", "", " Set the target build stage to build") | ||
| RootCmd.PersistentFlags().BoolVarP(&noPush, "no-push", "", false, "Do not push the image to the registry") | ||
| addSetupFlags(RootCmd) |
There was a problem hiding this comment.
Adding a level of indirection for flag setup is unusual, but clearly you have something in mind. Do you mind clarifying the intent?
There was a problem hiding this comment.
The flags in that function were meant to be ones that are used in the prerun (things like setting loglevel), but I might move them back to init() for clarity
pkg/executor/push.go
Outdated
| rt := &withUserAgent{t: tr} | ||
|
|
||
| if err := remote.Write(destRef, image, pushAuth, rt, remote.WriteOptions{}); err != nil { | ||
| logrus.Error(fmt.Errorf("Failed to push to destination %s", destination)) |
There was a problem hiding this comment.
Logging and error and propagating it upwards usually results in duplicate errors being logged. I generally suggest one or the other unless there is a good reason.
pkg/executor/push.go
Outdated
| // Push the image | ||
| destRef, err := name.NewTag(destination, name.WeakValidation) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
DoPush() has many code paths that can return an error. Do you mind adding some context to the errors returned by it to assist future debuggers? For instance:
"nametag failed: %v", err
I noticed other parts of the kaniko codebase use errors.Wrap for this.
There was a problem hiding this comment.
Yah I actually just added in errors.Wrap in this refactor to improve our error messages -- I'll definitely add it here but it will probably require another PR to fix all of our error messages.
In this refactor I:
passed in through the command line
a new file push.go
--context instead, and marked an aws flag as hidden which is set in a
vendored directory (not totally sure why but it shows up in the help text for the command)
Should fix documentation issue in #290