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 Tasks Documentation #5346

Closed
wants to merge 13 commits into from
Closed

Conversation

ncapps
Copy link
Contributor

@ncapps ncapps commented Sep 26, 2023

This change adds content to the Core Tasks section of the new documentation site.

Resolves: #5286

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

@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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 26, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @ncapps. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 26, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ncapps
Once this PR has been reviewed and has the lgtm label, please assign natasha41575 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 27, 2023
@ncapps
Copy link
Contributor Author

ncapps commented Sep 27, 2023

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 27, 2023
@ncapps ncapps marked this pull request as ready for review September 28, 2023 00:44
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2023
@natasha41575
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 28, 2023
Copy link
Member

@bugoverdose bugoverdose left a comment

Choose a reason for hiding this comment

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

I'm leaving just a few minor suggestions. Good work though.

site/content/en/docs/Tasks/configmaps_and_secrets.md Outdated Show resolved Hide resolved
Comment on lines 16 to 17

Copy link
Member

@bugoverdose bugoverdose Sep 29, 2023

Choose a reason for hiding this comment

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

In this sentence, the doc is referring to the users as if they are third party.
So it sort of makes as feel like this doc is for the contributors to read, not the actual users of kustomize. Same goes for all the other first sentences of Motivation.

Suggested change
Users may want to define a common set of labels or annotations for all the Resources in a project.
It's possible to define a common set of labels or annotations for all the Resources in a project.
Suggested change
Users may want to define a common set of labels or annotations for all the Resources in a project.
You can define a common set of labels or annotations for all the Resources in a project.

Comment on lines +15 to +16
## Motivation
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the section name Motivation is necessary.

Suggested change
## Motivation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Motivation header was copied from the original site here. All other Task pages include a Motivation section as an introduction to the concept.

Personally, I think it makes sense to keep this header but I can remove if you feel strongly that it is not helpful.

Copy link
Member

@bugoverdose bugoverdose Oct 1, 2023

Choose a reason for hiding this comment

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

Yeah, I think it's OK to keep the header if you think it's useful.

@natasha41575
Copy link
Contributor

I think the tests aren't triggering because of #5332, I'm going to revert that one today.

@ncapps ncapps requested a review from bugoverdose September 29, 2023 23:12
@annasong20
Copy link
Contributor

@ncapps Thanks so much for working on this section! You did exactly as the sheet instructed, which was to leave only the conceptual information in the "core_tasks" section.

Unfortunately, I didn't write this sheet, and didn't catch these comments. Sorry in advance. I found that the structure of the Tasks section in the Kubernetes documentation belies the excel sheet comments in that their Tasks section only documents concrete examples or as they call it, "a short sequence of steps". I wanted to discuss with you what our Tasks section should hold. If we follow the Kubernetes example, we can discuss on this PR where to move the content or work to merge this PR with the assumption that we'll move it in the future.

@ncapps
Copy link
Contributor Author

ncapps commented Sep 30, 2023

Hi @annasong20, Thanks for taking a look at my PR. Please let me know what you envision for the Kustomize Tasks section and I will update this PR accordingly.

@annasong20
Copy link
Contributor

Hi @ncapps, sorry for the delayed response. Also feel free to DM me on Slack if that's faster.

I don't have a great vision of all the sub-sections, but my reasoning was that in the absence of strong opinions, our site might be easier to understand if it follows the same format as Kubernetes. Their Tasks section seems to feature concrete steps for accomplishing simple workflows, aka Tasks. Taking configMapGenerator as an example,

  • the configuration example with .properties currently under Guides
  • all the different examples of the different sub-fields under Reference

seem like they belong under Tasks. The Reference looks like it should be used for quick lookups; I made the same belated comment on @bugoverdose's PR. For example, basic field syntax instead of containing examples. Motivation, introduction, background, reasoning, etc. seem like they should belong under Concepts, where I'd probably look to "understand" a topic.

I also think we might benefit from working on 1 or a few fields at a time. We can finalize our approach at a smaller scope and then you can apply it to all other fields. This way, we don't waste your time applying dissented changes between reviews.

Disclaimer that my take is not authoritative. Feel free to push back where you disagree! Treating this more like a collaboration.

@ncapps ncapps marked this pull request as draft October 6, 2023 00:49
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2023
@ncapps
Copy link
Contributor Author

ncapps commented Oct 15, 2023

I am closing the PR. Feedback will be addressed in #5368 and #5383.

@ncapps ncapps closed this Oct 15, 2023
@ncapps ncapps deleted the docs/tasks branch January 31, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
Development

Successfully merging this pull request may close these issues.

Document Tasks
5 participants