-
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
✨ Update component-config tutorial automatically #3191
✨ Update component-config tutorial automatically #3191
Conversation
Hi @Eileen-Yu. 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.
Nice job @Eileen-Yu 👍🏼 !
I did a first-round review and left some comments.
Will take another look later.
Thx for participating in contribution!
docs.sh
Outdated
|
||
source "test/common.sh" | ||
|
||
build_kb |
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.
we do not need to do those calls.
Whe we can add those via the Makefile right?
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.
Hi @camilamacedo86 I have a question here:
I can understand that if we run make generate
, the build_kb
will be loaded and kubebuilder would be built.
What I'm thinking is, if it's necessary to have a separate cmd, that can individually generate docs based on the code?
What we have now is make generate-docs
, that does not include build_kb
.
f50cb54
to
712ce9e
Compare
docs/docs.sh
Outdated
|
||
cd docs/book/src/component-config-tutorial/testdata/project | ||
|
||
make build |
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.
nits: what about merging the two lines:
(cd docs/book/src/component-config-tutorial/testdata/project; make build)
(Assuming in the future we may go to multiple tutorial directories and run the command..)
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.
We do not need the make build to do this one.
The build will be made in the ci already before we call make generate
Locally the contributor should work with make generate so, IMO we should keep the same behaviour.
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.
Do we have the step to execute codegen for those tutorials in the CI?
I failed to find the place. Do you have the reference?
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.
We call in the CI make check-testdata, see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/.github/workflows/testdata.yml#L27
Then, see that this target calls ./test/testdata/check.sh: https://github.com/kubernetes-sigs/kubebuilder/blob/master/Makefile#L120-L122
So, my thought, in the long run, is we have a makefile target that calls that run the go implementation to generate docs/ Also, add this one to make generate. And then, ensure that we are testing/checking for all PR changes if the docs were also generated/is updated accordingly.
Is that make sense? Such as we do in SDK to verify the samples as well.
...src/component-config-tutorial/testdata/project/config/manager/controller_manager_config.yaml
Outdated
Show resolved
Hide resolved
/ok-to-test That is a terrific contribution 🥇 |
fb8379c
to
2e70b94
Compare
docs/docs.sh
Outdated
|
||
cd docs/book/src/component-config-tutorial/testdata/project | ||
|
||
make build |
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.
@Eileen-Yu , why are we calling the make build in the shell?
We can call that via golang see, i.e. https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v4/plugin_cluster_test.go#L118
So, we can make("build") as well.
ac793c5
to
0be2b9a
Compare
.github/workflows/docs.yml
Outdated
run: sudo rm -f /usr/local/bin/kustomize | ||
- name: Verify docs | ||
run: | | ||
make generate-docs |
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.
See that CI has reported an error for missing kubebuilder
. That is possibly due to the execution of make generate-docs
without build_kb
.
I think we might have two approaches:
- In the new ci yaml file, add the setup to load and run
build_kb
, and then executemake generate-docs
. - Drop this new ci file, and add
generate-docs
tomake generate
.
WDYT? @Eileen-Yu @camilamacedo86
Well, I like the idea to have a separate pipeline focusing on docs validation.
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 think we should not use build_kb
and we should do == make generate and the make target and ci that check the testdata
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.
Hi @camilamacedo86
It makes sense to integrate the docs generation into check-the-testdata
, that can make the implementation a lot easier and straightforward.
Based on that, I'm concerned about some following points:
- If CI fails at
check the testdata
, will it become hard to tell at the first glance which part is wrong: eithertestdata
ORdocs
? - Would it be slow if we run all the generations in one pipeline versus having multiple pipelines for each generation?
- Is the priority of keeping
testdata
consistent higher thandocs
?
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.
It is OK to be another CI test. I am just saying that the implementation is very similar == to the target to generate the testdata samples and check it out.
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.
Hi @camilamacedo86 I just added CI for the docs check.
Could you pls help with review?
@Kavinjsir It sounds good to have multiple pipelines. How would you like to deal with this in some future PR?
Co-authored-by: Camila Macedo <[email protected]> chore: refactor
0be2b9a
to
0ba2830
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.
That is terrific work 🥇
@Kavinjsir I think all your comments are addressed. So, I am moving forward, but if you see a hall for improvements/changes, please track those issues, and we can try to address them in a fallow up.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, Eileen-Yu 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 |
Description:
Motivation:
Attempt to discuss the implementation and try to solve the first part of #2510