own up to the attrocities commited in set #19348
Conversation
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
size/L
|
@mfojtik do we have any tests on this? I'm surprised I got green out of the gate. |
|
/hold Needs tidying before merge |
openshift-ci-robot
added
the
do-not-merge/hold
deads2k
removed
the
do-not-merge/hold
|
tidying completed. Ready for review |
|
@deads2k can we just get rid of ResolveImage altogether and just call the patched resolve image? The factory function is exclusively used only by 'set' command. |
I don't think we should do that in this patch. I think that removing methods like that should be done upstream first. Otherwise we just end up carrying debt. |
| @@ -117,7 +122,7 @@ func (o *ImageOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []st | |||
| o.ChangeCause = f.Command(cmd, false) | |||
| o.DryRun = cmdutil.GetDryRunFlag(cmd) | |||
| o.Output = cmdutil.GetFlagString(cmd, "output") | |||
| o.ResolveImage = f.ResolveImage | |||
| o.ResolveImage = resolveImageFactory(f, cmd) | |||
There was a problem hiding this comment.
can we get rid of this field for set image and just call the function directly in Run() ? Also remove it from upstream factory as it is not used anywhere else.
|
@deads2k i'm fine with follow up (I can do it after this merges)... that shim was introduced by us by trying to plug our image resolution into upstream kubectl. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, mfojtik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
demonstrates what a mess we have.
We should never add flags again. This is needed to stop having a fake factory.
/assign @mfojtik