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

🌱 : Improve samples linting & fix samples lint issues #4548

Merged
merged 1 commit into from
Feb 10, 2025
Merged

🌱 : Improve samples linting & fix samples lint issues #4548

merged 1 commit into from
Feb 10, 2025

Conversation

wazery
Copy link
Contributor

@wazery wazery commented Feb 8, 2025 •
edited
Loading

✨ Summary

This PR enhances code quality and maintainability by addressing various linter issues and improving the CronJob controller Reconcile function code structure to reduce the cyclomatic complexity.

What's included in the PR:

  • Adds linting to documentation samples to ensure consistency and prevent future issues.
  • Fixes lint issues across multiple tutorials and utilities, improving code readability and compliance.
  • Refactors the CronJob controller’s Reconcile function for the samples to reduce cyclomatic complexity and enhance maintainability.
  • Consolidates sample linting into a single workflow for better efficiency and organization.

Motivation:

The goal of this PR is to enhance the overall linting coverage of sample code, ensuring that all included examples meet best practices. By addressing lint issues and integrating more samples, this change improves code consistency.

Fixes: #4445

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 8, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: wazery / name: Wazery (0c4ce6d)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 8, 2025
@k8s-ci-robot
Copy link
Contributor

Welcome @wazery!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @wazery. 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 8, 2025
@wazery
Copy link
Contributor Author

wazery commented Feb 8, 2025

I hope my changes to the cronjob_controller.go don't introduce any issues with the docs. I am trying to generate them and validate in the meantime.

camilamacedo86
camilamacedo86 reviewed Feb 8, 2025
View reviewed changes
docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/Makefile Outdated
@@ -1,3 +1,5 @@
# Image URL to use all building/pushing image targets
IMG ?= controller:latest
Copy link
Member

Choose a reason for hiding this comment

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

That’s interesting! 🤔

I think the plugin could definitely have more targets—many of them would be valuable here, though not all from the default scaffold.

Just a quick note: this sample is actually an external plugin, meaning it will be packaged as a binary that generates scaffolds, similar to Kubebuilder. However, it is designed to work alongside Kubebuilder to extend functionality rather than replace its core scaffolding. More details here: External Plugins.

Given that, I’d recommend reverting the changes in this PR.
If you’d like to improve the plugin by adding meaningful targets, that can definitely be done—but in a separate PR. We’ll also need to carefully evaluate which targets make sense for this context.

For example, we should not include targets like docker build for building a container image, because this project does not include a manager, Dockerfile, or containerized components, and it shouldn’t require them.

Hope this clarifies! Let me know your thoughts. 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that truly makes perfect sense now to me. Will update the PR accordingly, thanks for the review.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 8, 2025
@wazery
Copy link
Contributor Author

wazery commented Feb 8, 2025

@camilamacedo86 updated the PR accordingly now. Truly appreciated the kind guidance 🙇 .

@wazery wazery requested a review from camilamacedo86 February 8, 2025 23:34
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 8, 2025
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 9, 2025
camilamacedo86
camilamacedo86 reviewed Feb 10, 2025
View reviewed changes
docs/book/src/cronjob-tutorial/testdata/project/internal/controller/cronjob_controller.go Outdated
return ctrl.Result{}, err
}
if cronJob == nil {
return ctrl.Result{}, nil // CronJob deleted
}
Copy link
Member

Choose a reason for hiding this comment

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

We cannot remove the previous content or the comments it is part of your docs/tutorials.
We just need to address the lint issues but keep all as before.
What lint issue are you facing here?

camilamacedo86
camilamacedo86 requested changes Feb 10, 2025
View reviewed changes
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.

Hi @wazery

Great work 🥇
Just one thing here shows not accurate
We cannot change the docs/book/src/cronjob-tutorial/testdata/project/internal/controller/cronjob_controller.go at this away because it is used in the docs tutorial and those will broke it out.

I understand that the lint issue that you tried to solve is that the func has more than 30 lines, in this case we need add on top of the func // nolint:<name of lint> to ignore this case.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 10, 2025
@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 10, 2025 •
edited
Loading

/ok-to-test
/approve
/lgtm

Great work 🥇 🚀

Just I nit I will change the emoji because we use it to generate the release.
I will add 🌱 but since it is related to the docs could also be 📖

@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 Feb 10, 2025
@k8s-ci-robot k8s-ci-robot added 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 Feb 10, 2025
@camilamacedo86 camilamacedo86 changed the title ✨ Improve samples linting & fix samples lint issues 🌱 : Improve samples linting & fix samples lint issues Feb 10, 2025
@wazery
Copy link
Contributor Author

wazery commented Feb 10, 2025

I am not sure why the Testdata verification workflow fails, although when I run locally make check-docs it passes.

@camilamacedo86
Copy link
Member

Hi @wazery

I think it still not properly updated.
Can you please:

  • Update your branch with the master
  • Run make install && make generate
  • Commit the changes / squash and push?

The validation just run the commands and check if has a git diff.
If the results are not == then it fails.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2025
@wazery
Copy link
Contributor Author

wazery commented Feb 10, 2025

Hi @wazery

I think it still not properly updated. Can you please:

  • Update your branch with the master
  • Run make install && make generate
  • Commit the changes / squash and push?

The validation just run the commands and check if has a git diff. If the results are not == then it fails.

I recreated the branch on latest master, reran the make install & generate commands and committed. Hope now it passes.

@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2025
camilamacedo86
camilamacedo86 approved these changes Feb 10, 2025
View reviewed changes
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.

/lgtm

@camilamacedo86
Copy link
Member

Hi @wazery

The lint has failures
And looking the diff ijn the testdata, it does not seems that your branch has the latest updates introduced in the master branch. You need like

git checkout master
git pull master
git push

git checkout this-branch
git rebase master
make install
make generate

@wazery
🌱 Improve samples linting & fix samples lint issues
0c4ce6d 
This commit includes:
- Fix linter issues
- Refactor fetchCronJob in sample controller reconcile func
- Fix getting-started tutorial lint issues
- Fix multiversion tutorial lint issues
- Fix cronjob-tutorial lint issues
- Refactor cronjob controller reconcile to reduce cyclomatic complexity
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2025
@wazery
Copy link
Contributor Author

wazery commented Feb 10, 2025

@camilamacedo86 I fixed the issue, it was mainly that I introduced a run for goimports to reformat the file after generating it, but it seems this is an overkill. I removed that and rather fixed the format issue manually in the scripts so it will generate properly formatted code.

camilamacedo86
camilamacedo86 approved these changes Feb 10, 2025
View reviewed changes
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.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, wazery

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 merged commit 338af81 into kubernetes-sigs:master Feb 10, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@rashmigottipati rashmigottipati Awaiting requested review from rashmigottipati

Assignees

@camilamacedo86 camilamacedo86

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

Successfully merging this pull request may close these issues.

Add linter for the samples scaffolded under docs
3 participants
@wazery @k8s-ci-robot @camilamacedo86