-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add .yamllint config file to use yamllint to enhance linting for yaml files #1521
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yuanning6 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 |
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Welcome @yuanning6! |
Hi @yuanning6. 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. |
/cc @Prajyot-Parab |
/easycla |
/ok-to-test |
@yuanning6 Few Pointers -
|
@Prajyot-Parab Thank you for taking the review! I'll work on them after the busy finals :') |
@Prajyot-Parab I removed one rule which was to check the Document Start Marker from yaml files. Now it gives warning: "missing document start "---"", it used to give another warning when the Document Start Marker is not asked: "found forbidden document start "---"". Not sure whether this rule should be included or not, it gives a warning no matter what. To output only error level problems we can use Regarding the Makefile part, I'm not sure if I completely understood your requirements. Please let me know if there's anything I need to change! There's also a conflict: https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/pull/1521/conflicts. Do I have the permission to resolve it? |
@yuanning6: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Hi @Prajyot-Parab , hope you had a great holiday! I updated my branch to make it keep up with the main branch, but there's still the conflict. And After updating, it shows I want to merge 40 commits into main, I'm not sure whether something went wrong or not... t-t Could you please give me some guidance? Thank you!! |
Step 1 - You should be on your commit |
…enerated config dir, fixed linting issues seen
db52cde
to
0c4bfa7
Compare
Hi @Prajyot-Parab , I followed your instruction, it worked! Thank you so much! I would be very grateful if you could take a look at the code at your convenience. :)) |
@@ -150,6 +150,11 @@ vet: | |||
help: # Display this help | |||
@awk 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n\nTargets:\n"} /^[0-9A-Za-z_-]+:.*?##/ { printf " \033[36m%-45s\033[0m %s\n", $$1, $$2 } /^\$$\([0-9A-Za-z_-]+\):.*?##/ { gsub("_","-", $$1); printf " \033[36m%-45s\033[0m %s\n", tolower(substr($$1, 3, length($$1)-7)), $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) | |||
|
|||
# Install yamllint if not present |
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.
- Move this target under lint/verify section
- This will just install the lint (you can refer normal lint target in Makefile to see how the tooling is installed and follow same approach)
- You need to introduce a target to run the yamlint against the project (present one just installs it)
PR needs rebase. 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. |
@yuanning6 any update on this one. |
@Prajyot-Parab Sorry I'm a bit busy these days, will look into it next week! |
Hi @Prajyot-Parab ! I made another PR: #1727 (because I messed up with commits in the previous one...). This one has the targets to run yamllint outputting warnings level problems or not outputting. Please let me know if any improvements are needed! |
/close in favor of #1727 |
/close |
@Prajyot-Parab: Closed this PR. In response to this:
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. |
… files
What this PR does / why we need it:
This PR adds a .yamllint config file which has rules to config yamllint to enhance linting for yaml files. The lint will be helpful to avoid issues like 7cd7fcc.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1497
Special notes for your reviewer:
This is a quick implementation just to give an overview and directions, I will continuously make improvements and changes as needed.
To install and use yamllint: Quickstart
To see the behavior: Run
yamllint .
in the project root directory to lint all yaml files./area provider/ibmcloud
This PR doesn't change any image versions.
Release note: