This change is 

FYI @smarterclayton @pmorie @sjenning @jpeeler -- this makes getting logs via CLI for service catalog much simpler since we do not need to know the pod name.

Gotta love the "the code supported this all along, but for some reason we bailed on it; let's not do that." PRs 😄

So... I guess the assumption is that all pods that match the label selector have the named container? i.e. the selector only matches pods from a single replicaset's pod template most likely?

Otherwise I guess the command just chunks an error for each pod that doesn't have the specified container?

@sjenning -- the error message when the container is not found is as follows:

$ kubectl logs -l app=sc-catalog-apiserver --namespace=service-catalog --container=apiserver4
Error from server (BadRequest): container apiserver4 is not valid for pod sc-catalog-apiserver-4025205670-8xgb9

@derekwaynecarr and i guess you get one of those errors per pod that matches the selector but doesn't have the container.

either way, the code handles it as well as it can i think.

/lgtm

@kubernetes/sig-cli-pr-reviews fyi

Gotta love the "the code supported this all along, but for some reason we bailed on it; let's not do that." PRs

Eh, it really doesn't. Given a label selector which covers non-congruent pods, if you specify a container, should we search all pods to find the first one that has the container you're looking for (likely what you want), or do you want to take the first matching pod and fail if it doesn't have the container even if the second one does (what it actually does).

FYI @smarterclayton @pmorie @sjenning @jpeeler -- this makes getting logs via CLI for service catalog much simpler since we do not need to know the pod name.

kubectl logs deployment/service-catalog is probably what you're looking for.

kubectl logs deployment/service-catalog is probably what you're looking for.

There's one caveat of doing that - this will only pick the first pod in a deployment.

3. view a set of pods with a specific container's logs (-l foo=bar -c
   something)

Not having the container in the list is only a problem when you get no
output at all.

I don't object to updating to handle the case, but it's not just a matter of removing the validation check. The behavior needs to be defined or we'll have "bug" fixes in both directions.

how do you want to update the behavior versus that which is implied here? i guess we need a sorted pod selection order?

how do you want to update the behavior versus that which is implied here? i guess we need a sorted pod selection order?

Stable sorting and documenting "try first" to at least get consistent behavior running the same command multiple times against the same set of resources.

As a user, I'd kind of expect to having you look through to find the pod I meant, but if @kubernetes/sig-cli-api-reviews and @kubernetes/rh-ux are ok with failing instead of looking I'm ok with it.

@k8s-bot pull-kubernetes-unit test this

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, sjenning, wojtek-t

Seems like a small enough bugfix that reference an issue in the PR body is not required. Readding the approved label

@k8s-bot pull-kubernetes-e2e-kops-aws test this

@derekwaynecarr: The following test failed, say /retest to rerun them all:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

/retest

Automatic merge from submit-queue (batch tested with PRs 38751, 44282, 46382, 47603, 47606)