Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ephemeral containers to streamLocation name suggestions #81678

Merged
merged 1 commit into from Feb 13, 2020

Conversation

verb
Copy link
Contributor

@verb verb commented Aug 20, 2019

This combines container names into a single list because separating them into a long, variable length string isn't particularly useful in the context of a streaming error message. Continuing to conditionally append separate lists of strings would lead to the following potential error messages:

- a container name must be specified for pod test, choose one of: [container1 container2]
- a container name must be specified for pod test, choose one of: [container1 container2] or one of the init containers: [initcontainer1] or one of the ephemeral containers: [debugcontainer1]"
- a container name must be specified for pod test, choose one of: [container1 container2] or one of the init containers: [initcontainer1]
- a container name must be specified for pod test, choose one of: [container1 container2] or one of the ephemeral containers: [debugcontainer1]"

These long error messages with embedded lists make it hard to find container names at a glance, and the variable ordering makes it easy to mistake the container types. This isn't overly important since the container types aren't important in the context of someone who needs to type a container name in kubectl, but if the type is unimportant then we shouldn't display it in the first place.

The replacement error message proposed here displays all container names a single, consistently located list that is easy to find at a glance:

a container name must be specified for pod test, choose one of: [initcontainer1 container1 container2 debugger]

What type of PR is this?
/kind cleanup

What this PR does / why we need it: Ephemeral containers are available as a streaming location.

Which issue(s) this PR fixes:
WIP #27140

Special notes for your reviewer: I expect removing container type from this error message will be controversial, and I'm not married to it, but this is how I think it should be solved so let's start there.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 20, 2019
@verb
Copy link
Contributor Author

verb commented Aug 21, 2019

/retest

@verb
Copy link
Contributor Author

verb commented Aug 21, 2019

/sig node
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 21, 2019
@verb
Copy link
Contributor Author

verb commented Aug 23, 2019

/assign @tedyu

@tedyu
Copy link
Contributor

tedyu commented Aug 23, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2019
@verb
Copy link
Contributor Author

verb commented Aug 23, 2019

/assign @wojtek-t

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 27, 2020
@verb
Copy link
Contributor Author

verb commented Jan 28, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 28, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2020
@verb
Copy link
Contributor Author

verb commented Jan 29, 2020

/retest

1 similar comment
@verb
Copy link
Contributor Author

verb commented Jan 30, 2020

/retest

@verb
Copy link
Contributor Author

verb commented Jan 30, 2020

/retest

@verb
Copy link
Contributor Author

verb commented Jan 30, 2020

almost there.

/retest

@verb
Copy link
Contributor Author

verb commented Jan 30, 2020

@wojtek-t Are you available to approve this?

@wojtek-t
Copy link
Member

@verb Sorry - I missed that.

/approve

Please rebase and I will lgtm.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2020
This combines container names into a single list because separating them
into a long, variable length string isn't particularly useful in the
context of an streaming error message.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2020
@verb
Copy link
Contributor Author

verb commented Feb 12, 2020

@wojtek-t rebased, thanks!

@wojtek-t
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2020
@verb
Copy link
Contributor Author

verb commented Feb 12, 2020

/retest

1 similar comment
@verb
Copy link
Contributor Author

verb commented Feb 12, 2020

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit bd10420 into kubernetes:master Feb 13, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants