-
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
merge imagestreamtag list on patch #17091
Conversation
@smarterclayton @dgoodwin I don't think this is necessary for your ansible install issues, but it's probably a good idea. |
Tags []TagReference `json:"tags,omitempty" protobuf:"bytes,2,rep,name=tags"` | ||
// +patchMergeKey=name | ||
// +patchStrategy=merge | ||
Tags []TagReference `json:"tags,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,2,rep,name=tags"` |
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.
Can we add a test to test-cmd that verifies this merges correctly?
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.
done.
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.
Can we add a test to test-cmd that verifies this merges correctly?
Turns out, that was a really good idea.
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.
no need to encourage him...
e4fd2ca
to
3a5a100
Compare
/retest |
@smarterclayton test added. /retest |
/retest @smarterclayton bump. |
the thing that is being merged is an array. if the strategy was "replace" then the old elements would no longer be present. is there some third strategy? |
@smarterclayton done. |
/retest |
@smarterclayton i think this is ready now. |
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, smarterclayton 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 |
Automatic merge from submit-queue (batch tested with PRs 17149, 17091). |
@@ -272,7 +274,9 @@ type ImageStreamStatus struct { | |||
PublicDockerImageRepository string `json:"publicDockerImageRepository,omitempty" protobuf:"bytes,3,opt,name=publicDockerImageRepository"` | |||
// Tags are a historical record of images associated with each tag. The first entry in the | |||
// TagEvent array is the currently tagged image. | |||
Tags []NamedTagEventList `json:"tags,omitempty" protobuf:"bytes,2,rep,name=tags"` | |||
// +patchMergeKey=tag |
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.
well, that explains my issues. API changes after openshift/api was made :(
This will ensure we don't drop spec tags when applying a new set of imagestreams that is not a superset of the existing spec tags.
related to https://bugzilla.redhat.com/show_bug.cgi?id=1507031