Skip to content
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

Bug 1327108 - Updated error information when trying to import ImageStream pointing to a different one #9021

Merged
merged 1 commit into from Jun 15, 2016

Conversation

soltysh
Copy link
Member

@soltysh soltysh commented May 25, 2016

// disallow re-importing anything other than DockerImage
if existing.From != nil && existing.From.Kind != "DockerImage" {
return nil, nil, fmt.Errorf("tag %q points to existing %s %q, it cannot be re-imported", tag, existing.From.Kind, existing.From.Name)
}
// disallow changing an existing tag
if existing.From == nil || existing.From.Kind != "DockerImage" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the second part of the expression (beginning with ||) is no longer needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need for the second error message at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - fixed. And yes it is, see the attached bug.

// disallow changing an existing tag
if existing.From == nil || existing.From.Kind != "DockerImage" {
if existing.From == nil {
return nil, nil, fmt.Errorf("tag %q already exists - you must use the 'tag' command if you want to change the source to %q", tag, from)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting the error message from the mentioned bz:

error: tag "latest" already exists - you must use the 'tag' command if you want to change the source to ""

Since the from may be unset either strip the to %q suffix or make it appear depending on from.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but we lack information why you can't import it. One assumes if the tag is there it should be importable, which is not true for kind != DockerImage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed to address the problem upon next PRs that I'll be submitting when fixing import-image.

@miminar
Copy link

miminar commented May 25, 2016

One suggestion. LGTM

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2016
ImageStream pointing to a different one
@soltysh
Copy link
Member Author

soltysh commented Jun 14, 2016

Rebase and merging upon previous approval.

[merge]

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2016
@soltysh
Copy link
Member Author

soltysh commented Jun 14, 2016

Flake #9203.

[merge]

@smarterclayton
Copy link
Contributor

[merge] yum flake

@soltysh
Copy link
Member Author

soltysh commented Jun 15, 2016

Flake #9338.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 15, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4886/) (Image: devenv-rhel7_4380)

@soltysh
Copy link
Member Author

soltysh commented Jun 15, 2016

yum again...

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 4ab49ca

@openshift-bot openshift-bot merged commit b095e3a into openshift:master Jun 15, 2016
@soltysh soltysh deleted the bug1327108 branch June 16, 2016 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants