refactor registry command to Complete-Validate-Run #9236
Conversation
|
[test] |
pweil-
changed the title
|
re[test] |
| @@ -131,6 +141,9 @@ func NewCmdRegistry(f *clientcmd.Factory, parentName, name string, out io.Writer | |||
| Long: registryLong, | |||
| Example: fmt.Sprintf(registryExample, parentName, name), | |||
| Run: func(cmd *cobra.Command, args []string) { | |||
| kcmdutil.CheckErr(cfg.Complete(f, cmd, out, args)) | |||
| kcmdutil.CheckErr(cfg.Validate(cmd, args)) | |||
|
|
|||
| err := RunCmdRegistry(f, cmd, out, cfg, args) | |||
There was a problem hiding this comment.
This should be turned into kcmdutil.CheckErr(cfg.Run()) as well.
There was a problem hiding this comment.
The out here can be moved into RegistryConfig, args not needed at all.
There was a problem hiding this comment.
I didn't change it because of the ErrExit handling below which would change the overall behavior. I can look at that portion more
pweil-
force-pushed
the
registry-cmd-refactor
branch
from
f6de223 to
118f827
Compare
|
updated for feedback |
| nodeSelector map[string]string | ||
| ports []kapi.ContainerPort | ||
| namespace string | ||
| kClient *kclient.Client |
There was a problem hiding this comment.
Is this a service namespacer?
pweil-
force-pushed
the
registry-cmd-refactor
branch
2 times, most recently
from
799f9f3 to
6cac0cd
Compare
pweil-
force-pushed
the
registry-cmd-refactor
branch
from
6cac0cd to
abbf350
Compare
| image := cfg.ImageTemplate.ExpandOrDie(cfg.Type) | ||
| if ports, err := app.ContainerPortsFromString(opts.Config.Ports); err != nil { | ||
| return err | ||
| } else { |
|
Nits, overall LGTM. Thank you! |
pweil-
force-pushed
the
registry-cmd-refactor
branch
from
abbf350 to
9b5fa12
Compare
|
nits fixed |
|
@csrwng merge is at your disposal |
pweil-
force-pushed
the
registry-cmd-refactor
branch
from
9b5fa12 to
611214a
Compare
|
LGTM [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4938/) (Image: devenv-rhel7_4383) |
|
Evaluated for origin merge up to 611214a |
|
Evaluated for origin test up to 611214a |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4907/) |
|
yum flake #9203 [merge] |
|
|
@soltysh - for repayment of the Krowka candy you brought here is the refactoring I owed you.
Fixes #8390