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

Refresh kube-component reference docs #8283

Merged
merged 1 commit into from
May 3, 2018

Conversation

tengqm
Copy link
Contributor

@tengqm tengqm commented May 2, 2018

This PR updates the generated reference docs for kube-components.
The source used if from the 'release-1.10' branch of
kubernetes/kubernetes.

Close: #8280

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 2, 2018
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented May 2, 2018

Deploy preview for kubernetes-io-master-staging ready!

Built with commit d7e2234

https://deploy-preview-8283--kubernetes-io-master-staging.netlify.com

This PR updates the generated reference docs for kube-components.
The source used if from the 'release-1.10' branch of
kubernetes/kubernetes.
@Bradamant3
Copy link
Contributor

/assign

cc @steveperry-53

@tengqm This PR makes sense, except ... it removes the doc for a deprecated flag that isn't yet fully removed from the code/apiserver (--admission-control). We've now got --enable-admission-plugins properly documented (per issue #8280), but per discussion in PR #7887 we've lost the deprecated flag. And I don't understand the code base well enough to be able to figure out what's going on there. It looks to me as though on master, at https://github.com/kubernetes/kubernetes/blob/master/pkg/kubeapiserver/options/admission.go#L67ff, we should still get doc for --admission-control -- but that's not what's in the release-1.10 branch (code).

I won't hold up this PR for very long, but I'd like to find someone stateside who can explain the code to me, and maybe we can find some not terrible time that works for both of us to chat, too. I'm guessing these aren't the only flags that aren't showing up properly in the doc, but I'm using them as an indicator of the larger problem. Thanks for your patience!

@tengqm
Copy link
Contributor Author

tengqm commented May 2, 2018

@Bradamant3 I understand the concern for not showing the "hidden" flags. Those flags are not shown on the command line when you do, for example, kube-apiserver --help. Do you really think it makes senses to list an option in the reference documentation when that option disappeared from the command line? The whole idea of making flags "hidden" is stop advertising them to users. They are not completely removed simply because there are existing users who are still using them. This is my understanding why kubelet has hidden so many flags and kube-apiserver has also hidden a few of them. In my opinion, the "larger" problem you want to solve is at the upstream kubernetes/kubernetes project.

@Bradamant3
Copy link
Contributor

Bradamant3 commented May 2, 2018

@tengqm absolutely agreed that the larger problem needs to be solved upstream. But if you look at the master branch upstream, these lines are added (the second one NOT in release-1.10 branch), to show the doc for (at this this one) deprecated flag (https://github.com/kubernetes/kubernetes/blob/master/pkg/kubeapiserver/options/admission.go#L76 but also pasted here):

    fs.MarkDeprecated("admission-control", "Use --enable-admission-plugins or --disable-admission-plugins instead. Will be removed in a future version.")
    fs.Lookup("admission-control").Hidden = false

I'm not in a position to test atm, but this also looks to me as though there's at least meant to be some command-line help to indicate that the flag is deprecated. Which is good UX.

So now we (in docs land at least) are in a situation where we can't do the complete right thing if we generate from either master or release-1.10. And I'm guessing the problem exists (upstream) for other flags too, although the code is making my brain hurt trying to chase down just where --enable-admission-plugins lives. All sorts of bits, but it's based on a different model from --admission-control.

I'm overthinking, so assigning a couple of other folks whose judgment I trust to make the final call here.

@Bradamant3
Copy link
Contributor

/assign @heckj

/assign @chenopis

@tengqm
Copy link
Contributor Author

tengqm commented May 3, 2018

@Bradamant3 I don't buy in the idea of "hiding" flags at all. It smells like a lie to users. The components is doing things *undocumented". By "undocumented", I mean there are flags not shown on man pages, not meant to show as help messages, not meant to show as markdown docs, but these flags do exist secretly and they work! This design is somehow breaking the convention how things are deprecated (aka. deprecation policy) across the community.

@Bradamant3
Copy link
Contributor

@tengqm I agree. This line: fs.Lookup("admission-control").Hidden = false means the flag documentation is NOT hidden. (It's a weird pattern, and looks pretty brittle, but that's a separate issue.) The line, and therefore the doc, are missing in release-1.10. (There's also not a consistent pattern for implementing flags across the codebase, which doesn't help.) After Kubecon, I want to take the larger issue of flag deprecation and docs up with Jordan.

Not merging this PR yet bc the Travis build is broken this morning, but getting the process started bc better to document only the current flag (which is currently missing) than only the deprecated flag. Bad choice regardless, but there we are.

cc @liggitt

@Bradamant3
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2018
@liggitt
Copy link
Member

liggitt commented May 3, 2018

I think some form of these should be picked to 1.10.x to ensure we don't hide the flags that are currently usable:

@zacharysarah
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zacharysarah

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 May 3, 2018
@k8s-ci-robot k8s-ci-robot merged commit 258a6fd into kubernetes:master May 3, 2018
@tengqm tengqm deleted the kube-comp-ref branch May 6, 2018 03:28
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants