Skip to content

📖 Improve comments and fix typos for cronjob tutorial #4724

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chethanm99
Copy link
Contributor

Updated the files of cronjob_tutorial with minor grammatical changes, particularly api_design.go , controller_implementation.go , e2e_implementation.go , generate_cronjob.go , main_revisited.go , sample.go , writing_tests_controller.go, writing_tests_env.g to enhance readability and guide developers and contributors.

@k8s-ci-robot k8s-ci-robot added 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 Mar 31, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @chethanm99. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 31, 2025
@camilamacedo86 camilamacedo86 changed the title Minor changes to cronjob_tutorial files 📖 Improve comments and fix typos for cronjob tutorial Mar 31, 2025
@camilamacedo86
Copy link
Member

/ok-to-test
/approved
/lgtm

@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 Mar 31, 2025
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2025
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2025
@camilamacedo86
Copy link
Member

/lgtm cancel
/approve cancel

Please ensure the following steps are completed before we proceed with this PR:

  • The proposed grammar improvements must be reflected in the hack docs and properly updated in the tutorials using make generate-docs.
  • Run make generate-docs to ensure all sample projects are regenerated with the proposed changes.

Note: We generate the projects used in the tutorials, and then layer additional code through the hack/docs scripts.

We cannot move forward until these updates are properly applied.

Also, please squash your commits — this PR should contain only one commit for the final proposal.

Thank you!

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 31, 2025
@camilamacedo86 camilamacedo86 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2025
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Need to run make generate-docs to properly update the samples samples used in the docs with the changes.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chethanm99
Once this PR has been reviewed and has the lgtm label, please ask for approval from camilamacedo86. For more information see the 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

@chethanm99
Copy link
Contributor Author

Hi @camilamacedo86

Thanks for the guidance on running make generate-docs. I've been working through the errors and hit a blocker.

The command fails with:
ERRO error removing spec explanation: unable to find the content to be replaced

This happens in hack/docs/internal/multiversion-tutorial/generate_multiversion.go during the updateAPIV1 step. It's trying to replace the large comment block defined in the cronjobSpecComment variable within api/v1/cronjob_types.go.

However, the source file (docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_types.go) only contains the single line // CronJobSpec defines the desired state of CronJob. before the struct definition, not the large block the script expects.

I've verified that my commits didn't modify this source file (docs/.../api/v1/cronjob_types.go). It seems the script is trying to replace text that doesn't exist in the source material.

Could you please advise on how to proceed?

Thanks!

@camilamacedo86
Copy link
Member

ERRO error removing spec explanation: unable to find the content to be replaced

That happened because the changes broke the sample generation process.

We automatically generate the samples under the docs/ folder, then insert, replace, or update code based on predefined patterns.

It looks like you modified a text string (probably a constant) that comes from the default scaffold. Because of that, the generator can no longer find the original text to replace. It could also be due to a customization applied on top of previously generated code.

In this way, you need to find where the error is faced ( what is the replace that is not done, and why the original text is no longer found ) .. Let us know if you need a help with.

@chethanm99
Copy link
Contributor Author

Hi @camilamacedo86 ,

Thanks, I understand the process. I've done the investigation you suggested ("find where the error is faced... and why the original text is no longer found").

The specific replace that fails is in hack/docs/internal/multiversion-tutorial/generate_multiversion.go trying to remove the large text block defined in cronjobSpecComment from api/v1/cronjob_types.go.

The reason the text isn't found isn't due to my modifications to that file (I verified that docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_types.go was untouched by my commits), but because the source file itself only contains "CronJobSpec defines the desired state of CronJob" instead of the large block the script expects.

So, the script seems to be looking for text that doesn't exist in the source it's processing. How should I handle this discrepancy to get make generate-docs to pass for my PR?

@camilamacedo86
Copy link
Member

Hi @chethanm99,

Could you please ensure that your PR is rebased with the latest master branch?

After rebasing, please run:

make generate

The testdata check ensures that all sample projects and docs are properly generated. This validation runs for all PRs. So if the issue persists after rebasing, it's likely caused by changes in this specific PR.

Thanks a lot! I hope this helps you move forward. 🙌

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants