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 back defaulting for parameter decoding #49579

Merged
merged 2 commits into from Nov 16, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jul 25, 2017

At the beginning of 1.7, we removed the last "conversion causes defaulting". This broke the "default to true" behavior for exec and attach options, but we didn't notice. This removes the broken defaulter (you can default a non-point bool to true on an object) and adds back defaulting to parameter codecs.

@k8s-mirror-api-machinery-misc @lavalamp @smarterclayton

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 25, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 25, 2017
@ncdc
Copy link
Member

ncdc commented Jul 25, 2017

I believe that defaulting stdout and stderr to true has been a bug since the day it was introduced. What is the path forward to removing this behavior?

EDIT: To clarify a bit, if you do kubectl exec -it, the -t uses a tty, which means no data will ever be sent over stderr (stdout and stderr or muxed over the tty). I think this is a pretty common debugging use case, so I don't think it makes sense to default stderr to true. Defaulting stdout to true might be reasonable, as there are few use cases that I can think of where you would not enable it.

@deads2k
Copy link
Contributor Author

deads2k commented Jul 25, 2017

I believe that defaulting stdout and stderr to true has been a bug since the day it was introduced. What is the path forward to removing this behavior?

Well, since this doesn't work because of a mismatch between internal and external types results in getting a "real" New call. I think I agree.

@deads2k
Copy link
Contributor Author

deads2k commented Jul 25, 2017

Alright, I've manually confirmed that 1.6 always forced these values to true and that 1.7 respects them and does not apply any defaults.

There isn't a great way to have a fake defaulting path added in, so we'll need to sort out what to do.

@k8s-github-robot
Copy link

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", "release-note-experimental" or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 30, 2017
@k8s-github-robot
Copy link

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @deads2k @mikedanese @ncdc

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

@deads2k
Copy link
Contributor Author

deads2k commented Oct 24, 2017

We're going to want this.

@deads2k deads2k reopened this Oct 24, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017
@ncdc
Copy link
Member

ncdc commented Nov 14, 2017

@deads2k still want this?

@deads2k
Copy link
Contributor Author

deads2k commented Nov 14, 2017

@deads2k still want this?

It's been two releases, the world didn't explode. I'd say that the this reflects reality and we should fix defaulting in general.

@ncdc agree?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 14, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Nov 14, 2017

@deads2k still want this?

Yes. Rebased.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2017
@deads2k deads2k added release-note-label-needed release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 14, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 14, 2017

@deads2k: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce 85c49b2 link /test pull-kubernetes-federation-e2e-gce
pull-kubernetes-bazel 85c49b2 link /test pull-kubernetes-bazel

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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@deads2k
Copy link
Contributor Author

deads2k commented Nov 14, 2017

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Nov 15, 2017

@smarterclayton @liggitt for approval

@smarterclayton
Copy link
Contributor

/approve no-issue

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2017
@ncdc
Copy link
Member

ncdc commented Nov 15, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ncdc, smarterclayton

Associated issue requirement bypassed by: smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55798, 49579, 54862, 55188, 51990). If you want to cherry-pick this change to another branch, please follow the instructions here.

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants