Origin: oc returns empty list with -o yaml when resource is missing

[akostadinov]

oc started behave inconsistently returning empty resource list instead of error when option -o yaml is provided. This is a breaking change from before and inconsistent for scripting - i.e. script expects a Resource object or error, while now it gets an empty resource list.
Also it is inconsistent for command to return different error status depending on output format requested by user.

@pruan-rht , @liggitt , @ncdc

Version

oc v1.3.0-alpha.2-217-g1ca61fb

Steps To Reproduce

1. create project
2. oc get pod noonehere -o yaml
   

   Current Result

      apiVersion: v1
      items: []
      kind: List
      metadata: {}

Expected Result

 Error from server: pods "hello-openshift" not found

[ncdc]

Kubernetes commit bbdbfc89404f956ff96a06e999099765f5222a82, which is where origin master currently is, exhibits the same behavior. Kubernetes master (currently 594e4d883cca30e6d9a52862d5e4d5ea55f66af5) prints out the empty list AND it also displays the error. Talking to @liggitt it sounds like something has changed with the resource printers. Digging in now.

[ncdc]

Upstream issue https://github.com/kubernetes/kubernetes/issues/26466

Upstream partial fix https://github.com/kubernetes/kubernetes/pull/26686

[ncdc]

https://github.com/openshift/origin/pull/9578 pulls in upstream 26686. Reassigning to @smarterclayton to close this once his rebase lands

[liggitt]

Shouldn't the yaml output also be suppressed in an error case like that?

[ncdc]

@liggitt ideally, yes. See my comments here: https://github.com/kubernetes/kubernetes/pull/26686#issuecomment-229491977. If you return err instead of appending to allErrs, it suppresses the List. I know that https://github.com/kubernetes/kubernetes/pull/23673 was adding support for multi-resource processing, so I'm not sure if reverting back to an early return on error would affect that PR or not. But I'm in favor of the early return.

[ncdc]

@liggitt I opened https://github.com/kubernetes/kubernetes/issues/28243 for the empty list

[deads2k]

Shouldn't the yaml output also be suppressed in an error case like that?

Error case like what in particular? Off the cuff:

1. Request resource by name and fail?
2. Request N resources by name and some fail?
3. Request N resources by name and all fail?
4. Request N resource by name and list of M resources, all of the N's fail, the M's succeed but have no hits?

I think they should all return non-zero, they should all put output on stderr (the error).

I think that 2 and 4 should produce the list output they could retrieve. Doe we make life easier to harder by suppressing list output for 1 and 3?

[liggitt]

if all operations error, I would not expect an empty json/yaml list

[deads2k]

if all operations error, I would not expect an empty json/yaml list

@akostadinov this means that when you get a non-zero return, you will _sometimes_ have a list to interpret. Different scripts may choose to handle it differently.

I'm not against doing it, but having a "sometimes" rule always bothers me.

[akostadinov]

@deads2k , I don't think I have a use case that would break by what you say. But I also don't like "sometimes". ls for example lists what's available but also lists errors hit and exit status is non-zero. So if we output YAML with found items, it makes sense to have some list of errors as well. So script may check both - exit code and the error list when desired.

GCE API returns a list of errors from an operation. One can know if operation was partially completed.

[akostadinov]

The difference between one and multiple resources requested is for me data type. If user request one thing, getting a list of items is different data type. This is not a programming language but that is one thing bugging me in ruby, often API writers decide to return different data types in different conditions. And doing that in CLI also doesn't make sense to me.
Regardless of what we do for multiple resources requested, for single one, it is different IMO. Sorry I didn't understand your original concern.

[deads2k]

The difference between one and multiple resources requested is for me data type.

I have no issue with that.

[deads2k]

if all operations error, I would not expect an empty json/yaml list

it seems we're in agreement: https://github.com/kubernetes/kubernetes/issues/28243#issuecomment-229661997

[liggitt]

The difference between one and multiple resources requested is for me data type.

I have no issue with that.

I can't tell what is being suggested/agreed on

[ncdc]

Me either

[deads2k]

The difference between one and multiple resources requested is for me data type.
I have no issue with that.
I can't tell what is being suggested/agreed on

If you kubectl get single/resource, you should never get back a list. You should get back the flattened object or nothing.

[ncdc]

And what if you kubectl get pod/a pod/b?

[deads2k]

And what if you kubectl get pod/a pod/b?

In case 2: List (you requested multiple) printed to stdout, error to stderr, non-zero return

In case 3: Nothing to stdout, error to stderr, non-zero return

[liggitt]

If you kubectl get single/resource, you should never get back a list. You should get back the flattened object or nothing.

Isn't that what happens today?

[deads2k]

Isn't that what happens today?

Hence: "I have no issue with that".

[ncdc]

@liggitt no, it's not what happens today (not any more)

[liggitt]

in origin master:

$ oc get sa/default -o yaml
apiVersion: v1
imagePullSecrets:
- name: default-dockercfg-jqku9
kind: ServiceAccount
metadata:
  creationTimestamp: 2016-06-28T16:34:47Z
  name: default
  namespace: default
  resourceVersion: "261"
  selfLink: /api/v1/namespaces/default/serviceaccounts/default
  uid: 3a23e24e-3d4e-11e6-98ca-acbc32c1ca87
secrets:
- name: default-token-2tlix
- name: default-dockercfg-jqku9

does https://github.com/kubernetes/kubernetes/pull/26686 break that?

[ncdc]

Oh, I was referring to oc get pod/notfound -o yaml returning a list and rc=0.

[smarterclayton]

That is a bug. It should return no list, rc=1, and an error message to stdout

[ncdc]

Yeah, we have 2 potential upstream PRs to fix.

[liggitt]

@ncdc do we need to pick the upstreams for this?

[ncdc]

I'm pretty sure I already did the pick.

On Sunday, August 28, 2016, Jordan Liggitt [email protected] wrote:

@ncdc https://github.com/ncdc do we need to pick the upstreams for this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/issues/9639#issuecomment-243022793,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYhjzaQsrBTr3cel1vlDjyHWLkvRxks5qkk0FgaJpZM4JBijy
.

[smarterclayton]

This is fixed.