Allow setting patchStrategy for maps #12731
Conversation
pkg/image/api/v1/types.go
Outdated
| @@ -308,7 +308,7 @@ type ImageStreamTag struct { | |||
| Conditions []TagEventCondition `json:"conditions,omitempty" protobuf:"bytes,4,rep,name=conditions"` | |||
|
|
|||
| // Image associated with the ImageStream and tag. | |||
| Image Image `json:"image" protobuf:"bytes,5,opt,name=image"` | |||
| Image Image `json:"image" patchStrategy:"delete" protobuf:"bytes,5,opt,name=image"` | |||
There was a problem hiding this comment.
I wouldn't expect this to do good things. The delete directive is for when you building the patch, not for annotating the type, right? Type annotations are merge or replace as I recall.
There was a problem hiding this comment.
There are 3 directives from what I see. Although most of the time these are applied only to lists, including delete. Besides, here oc edit ens up producing a patch that is later on submitted to server to modify istag.
|
cc @liggitt |
|
this is must fix for 1.5 (otherwise you can't edit istags ;-) |
|
I don't think this is the right approach. See upstream issue kubernetes/kubernetes#20208 Let's coordinate there on what strategic merge patch should do for unknown or unmergeable fields, and see how reasonable it is to pick back to 1.5 |
soltysh
force-pushed
the
bug1403134
branch
3 times, most recently
from
f50858e
to
0d653ca
Compare
|
@liggitt I'm guessing the approach is approved upstream, only waiting for tests, do you think we can merge this as is upon green tests? |
|
[test] |
|
Updated the PR with the latest state of upstream PR, waiting for green tests and @liggitt approval |
|
Before acceptance we need following tests:
|
|
Evaluated for origin test up to e208952 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13740/) (Base Commit: 863f435) |
|
@liggitt wrt to the tests you've asked for, I can't do any of them, b/c the |
Sorry, I meant as unit tests, upstream. Making sure patch computation can completely remove a raw extension field, add one when missing, and replace one (you already have the last test) |
|
Roger that. |
|
upstream has LGTM, so this is ready as well |
|
LGTM (approved for 1.5) |
|
[merge] |
|
Evaluated for origin merge up to e208952 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/246/) (Base Commit: a0ea9b5) (Image: devenv-rhel7_5921) |
| os::cmd::expect_success_and_not_text 'oc get istag/test:new -o jsonpath={.metadata.annotations}' "tags:hidden" | ||
| editorfile="$(mktemp -d)/tmp-editor.sh" | ||
| echo '#!/bin/bash' > ${editorfile} | ||
| echo 'sed -i "s/^tag: null/tag:\n referencePolicy:\n type: Source/g" $1' >> ${editorfile} |
There was a problem hiding this comment.
I knew too much about image streams back then ;)
This is one possible approach to https://bugzilla.redhat.com/show_bug.cgi?id=1403134
ImageStreamTagallows editing only annotations, but that works only withoc annotateoroc patchcommand which specifically say what's edited. Whenoc editis being invoked it trips over the internalImageobject of a IST, due to differences between internal and external representations of this object.This approach allows setting
patchStrategyon struct objects, so they can be ignored, here deleted.@deads2k || @liggitt does something like that makes sense?
@juanvallejo fyi