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

ImageStreamTag update should ignore missing labels #15098

Merged
merged 1 commit into from
Jul 9, 2017

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jul 7, 2017

When we started returning labels for image stream tags in 3.6, old clients that
retrieved the tag and updated the field were broken if the image stream
had tags, since they would send empty labels (not having received them
via the API) on update, and labels are immutable.

This preserves the legacy behavior by defaulting the user provided image
stream tag labels to the image stream labels if no labels are set in the
image stream tag.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1463890

[test] @mfojtik @deads2k

When we started returning labels for image stream tags, old clients that
retrieved the tag and updated the field were broken if the image stream
had tags, since they would send empty labels (not having received them
via the API) on update, and labels are immutable.

This preserves the legacy behavior by defaulting the user provided image
stream tag labels to the image stream labels if no labels are set in the
image stream tag.
@smarterclayton smarterclayton added this to the 3.6.0 milestone Jul 7, 2017
@smarterclayton smarterclayton added area/compatibility component/image kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Jul 7, 2017
@smarterclayton
Copy link
Contributor Author

Supersedes #14803

@@ -219,6 +219,12 @@ func (r *REST) Update(ctx apirequest.Context, tagName string, objInfo rest.Updat
return nil, false, kapierrors.NewConflict(imageapi.Resource("imagestreamtags"), istag.Name, fmt.Errorf("another caller has updated the resource version to %s", imageStream.ResourceVersion))
}

// When we began returning image stream labels in 3.6, old clients that didn't need to send labels would be
// broken on update. Explicitly default labels if they are unset. We don't support mutation of labels on update.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ensured by a validator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, old clients were getting immutable errors (because they previously did not need to send labels even if the image stream had them).

@deads2k
Copy link
Contributor

deads2k commented Jul 7, 2017

One question. if it is ensured, then lgtm

@smarterclayton
Copy link
Contributor Author

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 0c65fbb

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3026/) (Base Commit: 5c7eff8) (PR Branch Commit: 0c65fbb)

@smarterclayton
Copy link
Contributor Author

Flake on networking setup, merge

@smarterclayton smarterclayton merged commit f1f1d60 into openshift:master Jul 9, 2017
@soltysh soltysh mentioned this pull request Aug 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/compatibility component/image kind/bug Categorizes issue or PR as related to a bug. priority/P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants