diff: Fix broken Local() logic
#61227
Conversation
k8s-ci-robot
added
release-note-none
|
/retest |
|
Test case so this doesn't revert in future? |
|
I probably could but:
|
|
Is there any chance this will make it into a minor 1.9 or 1.10 release? I'm really keen on this feature, and although i realise the implementation will have to change soon anyway, it'd be really nice to have in the interim. |
|
@toothbrush Actually, it won't have to change that much, so we should find a path forward at this point. I don't really have time to figure out how to test this, but I'd really like to get this merged too. @smarterclayton Any suggestions on how we can test this? :-) |
|
Why is mocking required? I was expecting a test-cmd scenario that prevents this regressing when we have server side apply |
Sounds good. Thanks! |
|
/uncc @dims |
|
I wish i could contribute more to this discussion than simply "how are we doing?" but i'm afraid that's all i've got! Maybe i should try my hand at constructing the test command? I would need pointers from the experts, though. |
|
I'll work on this tomorrow |
k8s-ci-robot
added
size/M
|
@smarterclayton I've added a test. It's not the best test in the world, but it would have caught this bug. |
|
/retest |
apelisse
force-pushed
the
fix-diff
branch
2 times, most recently
from
28683b5 to
a671add
Compare
k8s-ci-robot
added
size/L
pkg/kubectl/cmd/diff.go
Outdated
| info.Object = nil | ||
| remote, err := dl.Download(info) | ||
| if err != nil { | ||
| glog.Warningf("failed to download remote object: %v", err) |
There was a problem hiding this comment.
Nit but human warnings aren't via glog in CLIs, we use fmt.Fprintf(stdErr, "warning: ...")
glog is terrible for humans.
| } | ||
|
|
||
| func (d *Downloader) Download(info *resource.Info) (*unstructured.Unstructured, error) { | ||
| gvk := info.Object.GetObjectKind().GroupVersionKind() |
pkg/kubectl/cmd/diff.go
Outdated
| @@ -263,6 +266,7 @@ type Object interface { | |||
| // InfoObject is an implementation of the Object interface. It gets all | |||
| // the information from the Info object. | |||
| type InfoObject struct { | |||
| Remote *unstructured.Unstructured | |||
There was a problem hiding this comment.
adding remote here as unstructured explicitly seems inconsistent. I would have expected
Remote runtime.Unstructured
at a minimum.
Local and Live functions where doing and returning the same thing, giving empty results by default. Fix the local function by copying the objects before fetching the live version.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
Local and Live functions where doing and returning the same thing,
giving empty results by default. Fix the local function by copying the
objects before fetching the live version.
What this PR does / why we need it: Diff prints empty output by default. Fixes it.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #61145
Special notes for your reviewer:
Release note: