-
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
Add oc create imagestreamtag
#16224
Add oc create imagestreamtag
#16224
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.
I left you some comments.
} | ||
|
||
cmd.Flags().StringVar(&o.FromImage, "from-image", "", "Use the provided remote image with this tag.") | ||
cmd.Flags().StringVar(&o.From, "from", "", "Use the provided image stream tag or image stream image as the source: [<namespace>/]name[:<tag>|@<id>]") |
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.
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.
Yeah, I think having source
and from
is worse than from
and from-image
and from-X
. After all, that's consistent with oc create
in general for oc create secrets
which has from-file
and from-literal
.
I think in general source and from are first gen, while from
and from-X
are gen2
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.
What I was hoping is that we unify this across all the places, so that user whenever see flag from
or from-image
or else knows what can expect. Either this can be solved as part of this PR or an issue, please.
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.
But we can't break backwards compatibility, so it's more of a long running thing. I'd say a card is more appropriate, but we're not going to fix it anytime soon (i.e. i don't want to change tag or import-image right now, i'd want to ask "are we going to write an improved import-image that handles it")
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.
Card sgtm.
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.
I agree and I am aware of backwards compatibility :/ But at the same time it hurts our UX.
} | ||
|
||
func (o *CreateImageStreamTagOptions) Complete(cmd *cobra.Command, f *clientcmd.Factory, args []string) error { | ||
o.DryRun = cmdutil.GetFlagBool(cmd, "dry-run") |
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.
👍 for the dry-run, we need it in more places :)
test/cmd/images_tests.sh
Outdated
os::cmd::expect_success_and_text "oc get is/tag -o 'jsonpath={.spec.tags[?(@.name==\"7\")].referencePolicy}'" 'Local' | ||
os::cmd::expect_success_and_text "oc get is/tag -o 'jsonpath={.spec.tags[?(@.name==\"8\")].importPolicy.insecure}'" 'true' | ||
os::cmd::expect_success_and_text "oc get is/tag -o 'jsonpath={.spec.tags[?(@.name==\"9\")].importPolicy.scheduled}'" 'true' | ||
|
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.
I'm missing following tests:
- Create tag with default tag name (
latest
) applied automatically (iow.oc create istag mytag
) since I don't see any requirement on tag name being in the formrepo:tag
. - Create tag inside an existing IS, a
oc create is
+oc create istag
sequence would be nice. - Negative tests (cli validation faliures, creating already existing tag, etc).
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 don't do implicit latest here, adding a negative test. Will test inside.
03ea169
to
70967ff
Compare
Removed the old image policy tests since we are +4 versions out. |
Supports all spec tag options. Specify a source with --from (imagestreamtag) or --from-image (docker)
70967ff
to
860e49a
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton, soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
Flake #16248 |
@smarterclayton: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 16224, 14227) |
Supports all spec tag options. Specify a source with --from
(imagestreamtag) or --from-image (docker)