-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
📖 Documentation for go/v2 and go/v3 plugin #2569
📖 Documentation for go/v2 and go/v3 plugin #2569
Conversation
Welcome @Ashwin901! |
Hi @Ashwin901. 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 Once the patch is verified, the new status will be reflected by the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 🥇
Just a few nits commented in the PR
Also, see that we need to squash the commits to get this one merged.
Could you please do that?
Also, feel free to update improve the text suggestions as you see that fits
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be missing address the comments + rebase with master to solve the conflict with: #2562
Then we can get this one merged 🥇
Okay. I will do the changes and push it asap |
Hey @camilamacedo86 I made the necessary changes and also rebased with master. Can you let me know what the issue is? |
0b8ff9f
to
cacfd2d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another review round.
I think are closer now to getting this good enough to be merged
Great work !
cacfd2d
to
7149ba4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, just missing a few nits. Could you please address them so we are able to move forward?
Remove the bold and ensure that the notes has a title
Otherwise,
/approved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ashwin901, camilamacedo86 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 |
3eb61b1
to
90df9b9
Compare
@camilamacedo86 is everything fine now? Or are there any pending changes? |
Hi @Ashwin901,
It shows all fine for me. But I pinged in the channel to see if we get help on the review. |
<aside class="note"> | ||
|
||
<h1>But you can create your own plugins, see:</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the word "but"
Since you are contradicting a different paragraph, it's better just to write "You can create your own plugins, see"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hun, the page speaks about available plugins. then, could we not consider this content a note, or observation? Because of this in POV it shows fit better in the
|
||
- [Creating your own plugins][create-plugins]. | ||
- [Plugins Phase 2][plugins-phase-2] - will allow users to use these external options as helpers to perform the scaffolds with the tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This item should be outside to the aside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the two items should be placed outside the aside section?
|
||
</aside> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a link to the v3
</aside> | ||
|
||
[create-plugins]: creating-plugins.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line should probably be inside of the aside
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nops, the link alias should be at the bottom of the docs pages to make it easier we keep the maintained.
That is the standard.
|
||
[create-plugins]: creating-plugins.md | ||
[plugins-phase-2]: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/extensible-cli-and-scaffolding-plugins-phase-2.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line should probably be inside of the aside
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same ^
@@ -0,0 +1,57 @@ | |||
# go/v3 | |||
|
|||
Kubebuilder tool will scaffold the go/v3 plugin by default. This plugin is a composition of the plugins ` kustomize.common.kubebuilder.io/v1` and `base.go.kubebuilder.io/v3`. By using you can scaffold the default project which is a helper to construct sets of [controllers][controller-runtime]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Kubebuilder tool will scaffold the go/v3 plugin by default." -> add a new paragraph after this line, and perhaps make it bold.
"By using ..." -> "Using the plugin, you can..."
@@ -0,0 +1,51 @@ | |||
# go/v2 plugin | |||
|
|||
The `go/v2` plugin has the purpose to scaffold Golang projects to help users to build projects with [controllers][controller-runtime] and keep the backwards compatibility with the default scaffold made using Kubebuilder CLI `2.x.z` releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"...to scaffold Golang projects to help users..." -> "... to scaffold Golang projects by helping users..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"...to scaffold..." -> "...of scaffolding..."
|
||
</node> | ||
|
||
## When should I use this plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a question mark
Thank you for your contribution @Ashwin901 ! |
Hi @Ashwin901, mainly all suggestions are addressed and it shows in a good shape to get merged. So, @AlmogBaku @Ashwin901 Let's get this merged to not lost the contribution and work on some improvements shapes required as follows. I hope that is fine and I hope that you can help with this follow up so we are able to organize and improve the whole content. /lgtm |
/lgtm |
As mentioned in the issue #2542 I have added the documentation for the go/v2, go/v3 and also the available plugins page.
I have tried my best to follow all the checkpoints mentioned in the issue, but please let me know if any changes are required.