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.

Sign up for GitHub

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

Jump to bottom

Document generic data sources feature #19315

Merged
merged 2 commits into from Mar 17, 2020
Merged

Document generic data sources feature #19315

merged 2 commits into from Mar 17, 2020

Conversation

bswartz
Copy link
Contributor

@bswartz bswartz commented Feb 25, 2020 •
edited

Document the AnyVolumeDataSource feature gate. Don't include any user-facing documentation at this time, because the the feature is experimental and only relevant to developers.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 25, 2020
@bswartz bswartz mentioned this pull request Feb 25, 2020
Open
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Feb 25, 2020
@netlify
Copy link

netlify bot commented Feb 25, 2020 •
edited

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

Built with commit 1a3853a

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

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 5, 2020
@bswartz bswartz changed the title [WIP] Document generic data sources feature Document generic data sources feature Mar 5, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 5, 2020
@bswartz
Copy link
Contributor Author

bswartz commented Mar 6, 2020

/assign @msau42

@bswartz
Copy link
Contributor Author

bswartz commented Mar 6, 2020

Link to the relevant section of the preview:

https://deploy-preview-19315--kubernetes-io-master-staging.netlify.com/docs/concepts/storage/persistent-volumes/#data-population

msau42
msau42 reviewed Mar 6, 2020
View reviewed changes
Copy link
Member

@msau42 msau42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add the feature gate to https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates

content/en/docs/concepts/storage/persistent-volumes.md Outdated

Users may create their own data populator CRDs and write their own data populator controllers
to handle them. Such data populators are responsible for creating the persistent volume and
binding it to the PVC on their own.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be easily messed up and has been the cause of some security vulnerabilities in the past. Do we intend to have an example that people can follow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the hello-populator example from the original KEP. The plan was to present a formal design for how this should work and to get that approved as part of graduation to beta.

While writing these docs I realized how particularly unhelpful it is to have a feature gate with so little to say about it. Before beta we also need to address the issue of how users can find out what populators exist, and making sure to return good error messages when someone attempts to use something that's not installed.

Do you think it's better to say less? I don't want to encourage people to write code that will cause security problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah maybe until we have agreed upon pattern for populators we should not add a section here. Instead just document the feature gate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It removed this section and instead just documented the feature gate.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 10, 2020
@msau42
Copy link
Member

msau42 commented Mar 10, 2020

/lgtm

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

Hi @bswartz
We are very close to the docs final deadline i.e All 1.18 enhancements docs should be in ready to merge state by Monday 16th March. Please make sure to complete all the docs reviews before the deadline.

Thanks!

@bswartz
Copy link
Contributor Author

bswartz commented Mar 13, 2020

Hi @bswartz
We are very close to the docs final deadline i.e All 1.18 enhancements docs should be in ready to merge state by Monday 16th March. Please make sure to complete all the docs reviews before the deadline.

Thanks!

This is ready to merge. Just waiting for approval.

@jimangel
Copy link
Member

@bswartz please target the dev-1.18 branch for this PR.

/cc @VineethReddy02 @zacharysarah
/milestone 1.18

@k8s-ci-robot k8s-ci-robot added this to the 1.18 milestone Mar 16, 2020
@VineethReddy02
Copy link
Contributor

@bswartz as @jimangel mentioned please point this to dev-1.18 as these docs are specific to a feature enhancement targeted for 1.18 release.

@bswartz
Copy link
Contributor Author

bswartz commented Mar 16, 2020

@msau42 PR rebased on dev-1.18

@sftim
Copy link
Contributor

sftim commented Mar 16, 2020

@bswartz I think you'll need to change the base branch for this PR to dev-1.18; it's currently master.

For safety, I'm going to add a
/hold

Happy to run through details in case this is supposed to merge into master. https://kubernetes.io/docs/contribute/start/#choose-which-git-branch-to-use explains how SIG Docs uses branches.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2020
sftim
sftim reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown looks good to me.

If you're revising: here's an optional tweak you can make.

content/en/docs/reference/command-line-tools-reference/feature-gates.md Outdated Show resolved Hide resolved
@bswartz bswartz changed the base branch from master to dev-1.18 March 16, 2020 22:29
@bswartz @sftim
Update content/en/docs/reference/command-line-tools-reference/feature…
500c8e4 
…-gates.md

Co-Authored-By: Tim Bannister <tim@scalefactory.com>
@k8sio-netlify-preview-bot
Copy link
Collaborator

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 500c8e4

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5e6ffe2325406d0008790b1d

@bswartz
Copy link
Contributor Author

bswartz commented Mar 16, 2020

@sftim Thank you!

@sftim
Copy link
Contributor

sftim commented Mar 16, 2020

/remove- area blog
/remove-language de
/remove-language es
/remove-language id
/remove-language it
/remove-language ja
/remove-language ko
/remove-language pl
/remove-language zh

@k8s-ci-robot k8s-ci-robot removed language/de Issues or PRs related to German language language/es Issues or PRs related to Spanish language language/id Issues or PRs related to Indonesian language language/it Issues or PRs related to Italian language language/ja Issues or PRs related to Japanese language language/ko Issues or PRs related to Korean language language/pl Issues or PRs related to Polish language language/zh Issues or PRs related to Chinese language labels Mar 16, 2020
@sftim
Copy link
Contributor

sftim commented Mar 16, 2020

/remove-area blog

@k8s-ci-robot k8s-ci-robot removed the area/blog Issues or PRs related to the Kubernetes Blog subproject label Mar 16, 2020
sftim
sftim reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

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

Hey @bswartz , The changes look okay. I assume that this feature is a new addition for 1.18 or are you documenting an existing Alpha feature? Typically, there should be docs to introduce the feature.

@bswartz
Copy link
Contributor Author

bswartz commented Mar 17, 2020

Hey @bswartz , The changes look okay. I assume that this feature is a new addition for 1.18 or are you documenting an existing Alpha feature? Typically, there should be docs to introduce the feature.

@kbhawkey We decided to skip that for this feature since it's mostly developer facing (until beta) and any user-facing explanations would probably cause more problems then help. Earlier on I had a few paragraphs in the storage section, but I removed them after discussion with @msau42 .

@VineethReddy02
Copy link
Contributor

VineethReddy02 commented Mar 17, 2020 •
edited

/hold cancel
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VineethReddy02

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 Mar 17, 2020
@VineethReddy02 VineethReddy02 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit ca5b5c2 into kubernetes:dev-1.18 Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@msau42 msau42 msau42 left review comments

@kbhawkey kbhawkey Awaiting requested review from kbhawkey

@steveperry-53 steveperry-53 Awaiting requested review from steveperry-53

@VineethReddy02 VineethReddy02 Awaiting requested review from VineethReddy02

@zacharysarah zacharysarah Awaiting requested review from zacharysarah

Assignees

@sftim sftim

@msau42 msau42

@VineethReddy02 VineethReddy02

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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
1.18
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bswartz @msau42 @VineethReddy02 @jimangel @sftim @k8sio-netlify-preview-bot @kbhawkey @k8s-ci-robot