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 AnnotationsTransformer #5247

Merged
merged 5 commits into from
Sep 9, 2023
Merged

Document AnnotationsTransformer #5247

merged 5 commits into from
Sep 9, 2023

Conversation

annasong20
Copy link
Contributor

This PR aims to

  • setup the layout for documenting built-in transformers in Reference
  • document AnnotationsTransformer

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 21, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20

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 Jul 21, 2023
@annasong20 annasong20 force-pushed the document-annotations-transformer branch from 4c0da8b to 498656a Compare July 28, 2023 23:48
@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 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 Jul 28, 2023
@annasong20 annasong20 force-pushed the document-annotations-transformer branch from 498656a to 95b41df Compare July 29, 2023 00:07
@annasong20
Restructure existing Reference docs
18b16ed 
Restructure Reference section for site to better match k8s.io. Change
descriptions to complete sentences. Improve instructions to locally load
site.
@annasong20 annasong20 force-pushed the document-annotations-transformer branch from 95b41df to 8781e8b Compare August 4, 2023 18:33
@annasong20
Document AnnotationsTransformer on site
a91c40a 
Dcoument AnnotationsTransformer API under Reference on site.
@annasong20 annasong20 force-pushed the document-annotations-transformer branch from 8781e8b to a91c40a Compare August 4, 2023 20:09
@annasong20
Copy link
Contributor Author

/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 Aug 4, 2023
@annasong20 annasong20 marked this pull request as ready for review August 4, 2023 20:22
@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 Aug 4, 2023
@annasong20
Copy link
Contributor Author

/cc @natasha41575

@koba1t
Copy link
Member

koba1t commented Aug 17, 2023

/assign

This was referenced Aug 19, 2023
Closed
Closed
@koba1t
Copy link
Member

koba1t commented Aug 23, 2023

@annasong20

Those documents look pretty good!

I have one suggestion. Could we add a mark that this field is required or omittable on explains?
That is help to find more important fields in struct.

bugoverdose
bugoverdose reviewed Sep 1, 2023
View reviewed changes
site/README.md Outdated Show resolved Hide resolved
@annasong20
Document required fields
70fb197 
Document required fields and explain effects of optional ones.
@annasong20
Make site setup instructions more explicit
2e5a885
@annasong20 annasong20 force-pushed the document-annotations-transformer branch from 26368f6 to 2e5a885 Compare September 1, 2023 19:59
@annasong20
Copy link
Contributor Author

@koba1t Ready for another review!

I have one suggestion. Could we add a mark that this field is required or omittable on explains? That is help to find more important fields in struct.

Love the suggestion! There actually aren't required fields other than apiVersion, kind, and metadata for AnnotationsTransformer and fieldSpec, so I added a blurb about required Kubernetes fields. I also tried to explain the effects of not including each optional field. Lmk if I'm missing anything else!

@koba1t
Copy link
Member

koba1t commented Sep 5, 2023

@annasong20

I added a blurb about required Kubernetes fields. I also tried to explain the effects of not including each optional field.

Thanks! It looks great!
I think looks good to me. Please add the lgtm label when is a good time to merge.
Additionally, I added one below comment, but you will be able to ignore it.

super nit: If you think that is better to add the link for official doc that describes the required fields in Kubernetes, You can add the below link that I found.

@annasong20
Copy link
Contributor Author

@koba1t I added the link! If it's not too much trouble, would still prefer your lgtm as a final sanity check.

I also referenced this link when writing the original explanation, so it is definitely good to include.

@koba1t
Copy link
Member

koba1t commented Sep 9, 2023

Perfect!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2023
@annasong20
Copy link
Contributor Author

/test kustomize-presubmit-master

@k8s-ci-robot k8s-ci-robot merged commit 7c36ed2 into kubernetes-sigs:master Sep 9, 2023
9 checks passed
This was referenced Sep 23, 2023
Closed
Merged
Open
@annasong20 annasong20 mentioned this pull request Oct 12, 2023
Open
@annasong20 annasong20 mentioned this pull request Nov 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koba1t koba1t koba1t left review comments

@bugoverdose bugoverdose bugoverdose left review comments

@yuwenma yuwenma Awaiting requested review from yuwenma

@natasha41575 natasha41575 Awaiting requested review from natasha41575

Assignees

@koba1t koba1t

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/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
Kustomize Documentation
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@annasong20 @k8s-ci-robot @koba1t @bugoverdose