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

[mod] lint github YAML config files #3314

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

Conversation

return42
Copy link
Member

@return42 return42 commented Mar 9, 2024

What does this PR do?

yamllint github config files (YAML)

Why is this change important?

find issues before merge upstream into master branch

How to test this PR locally?

make test.yamllint

Related issues

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
@return42 return42 requested a review from dalf March 9, 2024 14:30
@dalf
Copy link
Member

dalf commented Mar 9, 2024

meh, I understand but the workflows are painful to test / develop. I often end up with git add .; git commit --ammend; git push --force until it works.
If the process crash because there is missing space, it will additional stress for no additional gain.
YMMV.

@return42
Copy link
Member Author

return42 commented Mar 9, 2024

meh, I understand but the workflows are painful to test / develop. I often end up with git add .; git commit --ammend; git push --force until it works.

Yeah, but you are talking about challenges in the development cycles when setting up a workflow, this PR is made to reduce avoidable errors / improve quality of YAML files.

If the process crash because there is missing space, it will additional stress

This yamllint test can be run before you create the commit and is started by the CI when we send a PR.

for no additional gain. YMMV.

The intention (gain) is to lint yaml files before merge commits into master (YAML files will be linted in the CI of a PR).

By example:

In #3313 I merged three YAML issues into the master tree --> https://github.com/searxng/searxng/actions/runs/8214791833/workflow#L117

To fix I needed two commits a48da9b and 691390b .. with the addition from this PR, YAML issues in the config files will be reported in the CI tests of a PR .. I would never had merged this yaml errors.

@dalf
Copy link
Member

dalf commented Mar 9, 2024

To fix I needed two commits a48da9b and 691390b .. with the addition from this PR, YAML issues in the config files will be reported in the CI tests of a PR .. I would never had merged this yaml errors.

I see these type of errors in my editor as I type.

This yamllint test can be run before you create the commit and is started by the CI when we send a PR.

It is going to do additional tests before the commit --> additional stress for nothing as long the editor make sure to show invalid YAML instantaneously.

@dalf
Copy link
Member

dalf commented Mar 9, 2024

About instant check, what is the purpose of these lines ?

searxng/.dir-locals.el

Lines 83 to 90 in 691390b

;; flycheck should use the local py3 environment
(setq-local flycheck-yaml-yamllint-executable
(expand-file-name "bin/yamllint" python-shell-virtualenv-root))
(setq-local flycheck-yamllintrc
(expand-file-name ".yamllint.yml" prj-root))
(flycheck-checker . yaml-yamllint)))))

Does it ask flycheck to run yaml-lint while you are editing?

@return42
Copy link
Member Author

return42 commented Mar 9, 2024

I see these type of errors in my editor as I type.

I see pyright and pylint issues in my editor but that is not a reason to remove these tests from the CI

additional stress for nothing as long the editor make sure to show invalid YAML instantaneously.

Sadly yaml-lint does not work yet well in my editor (emacs).

Does it ask flycheck to run yaml-lint while you are editing?

Yes, the intention is to set up the flycheck variables needed to start a yamllint in the emacs editor from the local python installation we have in local/py3/ (its like running the yammlint within a local/py3/bin/activate environment).

Sadly this setup does not work yet in my emacs .. 🤷

It is going to do additional tests before the commit --> additional stress

I don't have much experience with developing workflows, this PR would have helped me.

But if you think it unnecessarily complicates the development processes, then we can close the PR ... I will follow your lead, you have more experience than I do ..

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

Successfully merging this pull request may close these issues.

None yet

2 participants