Volume strategic merge #11062
Volume strategic merge #11062
Conversation
|
@openshift/cli-review @deads2k |
|
[test] |
|
Needs tests. |
|
@fabianofranz okay I am going to add some unit tests. Or did you mean some e2e tests too? |
|
please hold on from merging this until I add some more tests. |
|
@gnufied unit and if possible something in |
|
@fabianofranz PTAL, I have added unit tests for this. I do not think it is easily doable to further test this via |
| return fakeMapping | ||
| } | ||
|
|
||
| func TestVolumeRunWithPatch(t *testing.T) { |
There was a problem hiding this comment.
I would appreciate a test both for add and remove, this one does remove only.
There was a problem hiding this comment.
okay, will add. But partly It was intentional, the add path requires mocked objects which are rather complicated to fake/mock. :(
There was a problem hiding this comment.
Added a test for the scenario when volume is added as well.
|
@deads2k could you give this one a pass? |
| if v.List { | ||
| listingErrors := v.printVolumes(infos) | ||
| if len(listingErrors) > 0 { | ||
| return cmdutil.ErrExit |
There was a problem hiding this comment.
why aren't errors reported here?
There was a problem hiding this comment.
I could move error printing to here but since v.printVolumes already prints the errors, I did not bother.
|
I didn't go super deep, but it looks reasonable. |
|
@jsafrane Added a test case for add volume scenario as well. PTAL. |
|
Could use a squash. |
Right, holding merge until it gets squashed. |
gnufied
force-pushed
the
volume-strategic-merge
branch
2 times, most recently
from
a7650c3 to
9e00dd4
Compare
|
@fabianofranz @deads2k Okay, I have squashed the commits. |
|
LGTM [merge] |
|
[test] |
|
The error here looks like a flake. |
This adds support for using PATCH method when adding or removing a volume from openshift asset (deployment configuration). Doing so will help prevent future race conditions and brings volume command on par with other similar command that use PATCH.
gnufied
force-pushed
the
volume-strategic-merge
branch
from
9e00dd4 to
f513896
Compare
|
Evaluated for origin test up to f513896 |
|
Flaked on #11094 |
|
Evaluated for origin merge up to f513896 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9671/) |
1 similar comment
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9671/) |
|
@fabianofranz @childsb while I am looking at #11094 do we want to block this PR from getting merged? |
|
@gnufied no, that's a flake unrelated to this PR. Working on it does not prevent this from getting merged. |
|
#11094 [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9770/) (Image: devenv-rhel7_5150) |
Implement volume request using strategic merge patch. Fixes #4351