-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
unified printing strategy #5334
unified printing strategy #5334
Conversation
[test] me |
386183d
to
b2a4b16
Compare
}, | ||
} | ||
|
||
kcmdutil.AddOutputFlagsForMutation(cmd) |
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.
refactor this method upstream to bind to a variable.
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.
@deads2k what benefits from that change do I use to justify that PR?
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.
@deads2k what benefits from that change do I use to justify that PR?
Currently, the printer and output methods on the Factory
are very difficult to use during delegation flows. They rely on having a cmd
set up with exactly the "right" flags. We should refactor the Factory
to specify the actual values they need.
Having the proper object bound in will allow you separate the data from its expression.
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.
@deads2k thanks
@@ -78,6 +78,9 @@ func NewCmdStartBuild(fullName string, f *clientcmd.Factory, out io.Writer) *cob | |||
|
|||
cmd.Flags().String("git-post-receive", "", "The contents of the post-receive hook to trigger a build") | |||
cmd.Flags().String("git-repository", "", "The path to the git repository for post-receive; defaults to the current directory") | |||
|
|||
cmdutil.AddOutputFlagsForMutation(cmd) |
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.
Omit start build from this pr because this breaks a lot of examples and tests that expect the output to be the name only. We'll need a more nuanced solution there.
2b17e27
to
4daf41d
Compare
@deads2k I haven't made upstream changes to |
4daf41d
to
24df569
Compare
[test] |
1f70f77
to
a0d1048
Compare
re[test] |
a0d1048
to
4ce7574
Compare
legit failure this time. |
Jenkins cache cleared, re[test] |
4ce7574
to
b293b60
Compare
imagestream flake here:
|
@stevekuznetsov It's almost certainly because @deads2k and myself had killed a few jobs by hand yesterday and didn't clean everything up fully. This was the one that passed: https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/224/ |
re[test] |
flake on
|
UP, @stevekuznetsov please rebase and let's try to get this in. |
b293b60
to
65558ac
Compare
@fabianofranz this is broken, blocked on |
Evaluated for origin test up to 65558ac |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1578/) (Extended Tests: ldap_groups) |
Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again |
Closing this as I don't think it's really relevant anymore ... @fabianofranz feel free to pick this up if it is |
Fixes #5332
Fixes #5256
Updates the following commands to use
cmdutil.PrintSuccess
:openshift admin new-project
openshift admin groups new
openshift cli new-project
(TODO)openshift cli start-build
openshift cli secrets new
openshift cli secrets new-dockercfg
openshift cli secrets new-basicauth
openshift cli secrets new-sshauth
openshift ex sync-groups
@deads2k