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

[DX/tests] - Update doc tutorials programatically for any PR change #2510

Closed
2 of 3 tasks
camilamacedo86 opened this issue Feb 13, 2022 · 20 comments
Closed
2 of 3 tasks
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 13, 2022

What do you want to happen?

Description

Currently, we have the following samples which are used in the docs:

You can see that the docs are using the samples and the info provided on them are comments in the source code generated.
However, it has been very thought we keep updated with the changes performed in the scaffolds. Currently, we need to:

  • Run the helpers scripts under it samples dir (generate_cronjob.sh, generate_multiversion.sh, generate_componentconfig.sh )
  • Run make all
  • Then, compare the local branch with the master branch and then re-add the missing files and all code generated for the samples as the comments back
  • Run make all again to ensure and compare again to see if we are not missing anything

Goal

The goal of this task is we programmatically ensure that all samples will be updated for any pull request sent to the project. It. will help us keep the docs and update and improve the DX for contributors. Also, it can help us check the changes in the review process which also help us to easily ensure them.

Note that we have a shell script generate.sh which is called via the makefile target [make generate] (https://github.com/kubernetes-sigs/kubebuilder/blob/master/Makefile#L65-L67) to re-gen all samples under the testdata dir.

See that in the CI we check if the samples are updated or not and we do not lot contributions that change the scaffold go forward and get merged without the authors ensuring that they executed the make generate locally to keep all updated. (You can check the GitHub Action: https://github.com/kubernetes-sigs/kubebuilder/blob/master/.github/workflows/testdata.yml)

Proposed Solution

Note that we do something very similar in the SDK project using the same implementations to ensure that is samples under testdatawill have the required code changes. See: https://github.com/operator-framework/operator-sdk/blob/master/hack/generate/samples/internal/go/v3/memcached_with_webhooks.go

  • We also need to call this implementation in the make generate target to ensure that it will be re-gen as tested in the CI.

Extra info

  • We can have one PR to implement each book data to split the required work and make it easier
  • I'd recommend we get started by automating the Component Config because this one is a small project in the book
  • Be aware that for the samples CronJob Tutorial and Multiversion-tutorial the shell scripts generate_cronjob.sh, generate_multiversion.sh are missing steps and we usually need to add files back. In this case, would be nice to follow all the whole docs/tutorials and ensure that we are following the exact same steps that you are asking users to do. It will also help us ensure our guidance and please feel free to push PR's contributions to the docs if you find any issue with them.

Extra Labels

No response

Implementation solution for: #782

Current Status

@camilamacedo86 camilamacedo86 added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 13, 2022
@camilamacedo86 camilamacedo86 changed the title Update doc tutorials for any PR change [DX/tests] - Update doc tutorials for any PR change Feb 13, 2022
@camilamacedo86 camilamacedo86 changed the title [DX/tests] - Update doc tutorials for any PR change [DX/tests] - Update doc tutorials programatically for any PR change Feb 13, 2022
@camilamacedo86 camilamacedo86 added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Feb 13, 2022
@jmrodri
Copy link
Contributor

jmrodri commented Feb 24, 2022

This proposal makes sense to me. Anything that will improve keeping the samples updated is great.

@camilamacedo86 camilamacedo86 added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Feb 24, 2022
@shuheiktgw
Copy link
Contributor

Hi @camilamacedo86, can I work on this issue as my first contribution to kubebuilder project? 🙏

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Feb 24, 2022

Sure, I'd recommend we begin by trying. to check this approach with the Component Config because this one is a smaller sample the docs
.

@shuheiktgw
Copy link
Contributor

/assign

@shuheiktgw
Copy link
Contributor

Sorry that I could not find time to work on this for the past few weeks, but I've finally started working on it 🙇

@camilamacedo86
Copy link
Member Author

That is fine. No worry.

@shuheiktgw
Copy link
Contributor

Sorry, let me clarify what we need to do here in the component-config's case as an example!

Then, we use the utils to replace and insert the code source of the book samples as its comments back.

I'm not sure where "the code source of the book samples" is located... Would you give me some pointers to it?

@AlmogBaku
Copy link
Member

@shuheiktgw
If I understood correctly:

# enter the project
cd <kubebuilder>
# enter the docs src
cd docs/book/src/
# create testdata for each example
cd cronjob-tutorial/testdata/
./generate_cronjob.sh
cd ../../multiversion-tutorial/testdata/
./generate_multiversion.sh
cd ../../component-config-tutorial/testdata/
./generate_componentconfig.sh
# manually add/modify missing files and comments
cd ../../
git dif
echo "good luck with that! this is actually the hard task we trying to automate with the replace"
# remake all to ensure we didn't break the examples
cd cronjob-tutorial/testdata/ && make all && cd ../..
cd multiversion-tutorial/testdata/ && make all && cd ../..
cd component-config-tutoria/testdata/ && make all && cd ../..
# regenerate the proje
cd ../../../
make generate
echo "well done. clap yourself 👏🏽! and wish good luck for your reviewer"

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Apr 8, 2022

Hi @AlmogBaku,

If you are looking to know if that are the steps required to update the samples inside of the doc:
Mainly yes.

  • You do not need to run make generate: make generate generates the testdata samples based on the source code changes. In this case, you are just re-scaffolding the samples used in the doc and re-adding all examples/code and comments on top again.
    PS.: If you are looking to do this process I'd recommend you do project by project and not as you added ( first re-scaffold the cronjob, add bake all examples on top again and etc .. then move to the next one.

If you are asking if the automation will be like a shell script doing these steps:
No. That is not the idea or goal.
The proposed solution for automation will perform the steps and add the code/examples on top again very similar how we create the e2e tests in the project and as we do that in the SDK.

@camilamacedo86
Copy link
Member Author

Hi @shuheiktgw,

Are you working in this one?
If not, could you please unsigned so it can let others try to achieve this goa?

@camilamacedo86
Copy link
Member Author

Hi @ shuheiktgw,

Are you working on this one? If you need help please feel free to ping your questions into the kubebuilder slack channel. If not, could you please remove your assign so that others might want to looking on this one?

@shuheiktgw
Copy link
Contributor

Sorry that I didn't notice the message and I'm not working on this issue so let me unassign myself 🙇
/unassign

@Kavinjsir
Copy link
Contributor

@camilamacedo86 Thx for bringing this great idea! I'm trying to understand the workflow you suggest and list as below, would you like to fix me if anything wrong?

Idea

In brief, we hope every PR can keep the sample sync with the kubebuilder behavior.


Similar Scenario

Such attempt is similar as how we test the kubebuilder behavior through CI:

  1. For each uploaded PR, the CI is triggered to execute make generate.
  2. make generate removes /testdata and re-generate it.
  3. After make generate is finished, CI would then check if the content in /testdata is the same as what it is on the current branch. (git diff)
  4. If not, it means the kb behavior is changed in the PR, and the corresponding testdata is not sync with it.
  5. The author of the PR should then run make generate locally to verify that the proposed kb behavior works on test data.

Proposed workflow

So, what proposed in the issue is: apart from /testdata, we would also like to see /docs with the corresponding samples in its sub-folder can be synced with every PR:

  1. The author of the PR should update /docs (OR verify if there is any necessity to update /docs) based on his/her original code updates in the PR.
  2. If there is any changes to the /docs, they should be included in the commit.
  3. The CI should have additional steps to verify if /docs keeps sync with the updates of the PR.

Challenges

  1. We need to initialize a new script that jumps between different folders under /docs to run various scripts to overwrite the files of the samples.
  2. We may depend on the existing scripts (./generate_componentconfig.sh, ./generate_cronjob.sh), however, it is necessary to review these scripts:
    • Are they following the documentation?
    • Are they following the current main version of Kubebuilder?
  3. We need to add new workflows in github action to run additional steps during CI to check /docs

@Sajiyah-Salat
Copy link
Contributor

Thanks @Kavinjsir for your brief answer. I got some idea on how things work and what needs to change. Hey @camilamacedo86 is thier anything to add in @Kavinjsir comment.

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Jan 26, 2023

HI @Kavinjsir,

That #2510 (comment) is a great summary 🥇 . The proposed solution to sort out the challenges would be to do scaffolds as operator-sdk does (using kb as lib) to generate the samples with code on top, see:

Therefore, we would no longer need the shell scripts. We would call kubebuilder binary, run the commands to generate the docs samples, then in golang inject the code on top ( using the methods to insert, and replace the string as we do in the plugins. example to insert code in the controller )

@Eileen-Yu
Copy link
Member

HI @Kavinjsir,

That #2510 (comment) is a great summary 🥇 . The proposed solution to sort out the challenges would be to do scaffolds as operator-sdk does (using kb as lib) to generate the samples with code on top, see:

Therefore, we would no longer need the shell scripts. We would call kubebuilder binary, run the commands to generate the docs samples, then in golang inject the code on top ( using the methods to insert, and replace the string as we do in the plugins. example to insert code in the controller )

Thx @camilamacedo86 , just initialized a PR to start with the component-config, could you help with review?

@Nageshbansal Nageshbansal removed their assignment Feb 5, 2023
@Eileen-Yu
Copy link
Member

/assign

@camilamacedo86
Copy link
Member Author

For we close this one we just need start to generate the Multiversion-tutorial see: https://github.com/kubernetes-sigs/kubebuilder/tree/master/docs/book/src/multiversion-tutorial/testdata/project as we do that for others now, more info: https://github.com/kubernetes-sigs/kubebuilder/tree/master/hack/docs

A help here is very welcome since if we be able to do that it will reduce a lot our effort to keep the things maintained and also help us to better ensure the project and that our samples are still working basically after the changes.

@camilamacedo86 camilamacedo86 added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 1, 2024
@camilamacedo86
Copy link
Member Author

Hi @Eileen-Yu

You did an amazing job here. So, because it is only missing one tutorial to make easier others understand what we need to do I create an issue with all info here: #3880

If you wish to contribute within we will be more than happy to receive your contribution.
therefore, we are closing this one as done and in favor of : #3880

@Eileen-Yu
Copy link
Member

yeah the most work of this task is done except for 1 tutorial.
My apology for not having sufficient time now to finish all of them, but I'll be super glad to help with review if anyone would like to contribute to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

8 participants