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

[ci] add yaml files linter to pre-commit hook but skip some rules for now (Part 1) #6763

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

StrikerRUS
Copy link
Collaborator

First PR in series of PRs for linting yaml files.

See #6758 for background and future PRs' content required to fix all errors with the proposed config.

List of generated errors with the proposed config: https://github.com/microsoft/LightGBM/actions/runs/12383831332/job/34567296571#step:3:886.

List of available linter rules: https://yamllint.readthedocs.io/en/stable/rules.html.

Won't merge this PR without reviews from @borchero and @jameslamb (#6758 (comment)).

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I just proposed a different approach, would like to hear your thoughts.

rev: v1.35.1
hooks:
- id: yamllint
args: ["--strict"]
Copy link
Collaborator

@jameslamb jameslamb Dec 18, 2024

Choose a reason for hiding this comment

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

This will be a bit disruptive to local development.. running pre-commit run --all-files locally (which I do many times during local development)... will produce a lot of output and fail until we've fixed all the issues.

Would you consider the following instead?

  • add all the rules that currently produce errors to .yamllint.yml in this PR with :disable
  • remove the SKIP=yamllint in lint-python-bash.sh
  • enable more and more rules over each of the next set of PRs.

That would prevent any new issues from being introduced on master while allowing pre-commit run --all-files to continue working locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense! Thank you very much for the suggestion! I just pushed 4adf207 with these changes.

@StrikerRUS StrikerRUS requested a review from jameslamb December 19, 2024 21:39
@StrikerRUS StrikerRUS changed the title [ci] add yaml files linter to pre-commit hook but skip it for now (Part 1) [ci] add yaml files linter to pre-commit hook but skip some rules for now (Part 1) Dec 19, 2024
@StrikerRUS
Copy link
Collaborator Author

@borchero Could you please take a look when have time?

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thank you!

@StrikerRUS
Copy link
Collaborator Author

Trying to close-reopen for Azure restart.

@StrikerRUS StrikerRUS closed this Dec 24, 2024
@StrikerRUS StrikerRUS reopened this Dec 24, 2024
Copy link
Collaborator

@borchero borchero left a comment

Choose a reason for hiding this comment

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

LGTM! The approach outlined by @jameslamb (i.e. enable more rules in follow-up PRs) makes a lot of sense to me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants