[DO NOT MERGE][ci] add yaml files linter to pre-commit hook #6758
Conversation
| if: | | ||
| github.event.issue.pull_request && | ||
| contains('OWNER,MEMBER,COLLABORATOR', github.event.comment.author_association) && | ||
| startsWith(github.event.comment.body, '/gha run') |
There was a problem hiding this comment.
|
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
StrikerRUS
requested review from
guolinke,
jameslamb,
shiyu1994,
jmoralez and
borchero
as code owners
I'd prefer I think |
There was a problem hiding this comment.
In general, I support this! Left a few comments on areas where I think we should make a different choice about certain warnings.
And I agree with your statement from the PR description... multiple PRs would be better, given how much this is touching and that fact that (correct me if I'm wrong) yamllint does not do auto-fixes and these changes were made manually.
| $GITHUB_WORKSPACE/.ci/set-commit-status.sh \ | ||
| "${{ github.workflow }}" \ | ||
| "pending" \ | ||
| "${{ github.event.client_payload.pr_sha }}" |
There was a problem hiding this comment.
I like these types of splits for calling scripts / commands with 1 argument per line... let's keep them regardless of how we handle other line-length warnings.
| R_LIB_PATH=~/Rlib | ||
| R_LIBS="c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl')" | ||
| R_DEPS="c('Depends', 'Imports', 'LinkingTo')" | ||
| CRAN_MIRROR="https://cran.rstudio.com" | ||
| INSTALLATION_STR="install.packages(${R_LIBS}, lib = '${R_LIB_PATH}', " | ||
| INSTALLATION_STR+="dependencies = ${R_DEPS}, repos = '${CRAN_MIRROR}', Ncpus = parallel::detectCores())" |
There was a problem hiding this comment.
I think stuff like this is really hard to read, relative to the form that's a single install.packages().
What do you think about just ignoring line-length warnings from yamllint to start (or putting the limit very high, like 250 characters) and only focusing on indentation and correctness?
There was a problem hiding this comment.
I think that we shouldn't place bash scripts more than 10 (just an example value) lines length into configuration yaml files. Each such piece of code should have its' own .sh file [branch].
For now, we can just ignore this line.
I agree with @jameslamb . I think |
TBH, I just didn't checked possibility of auto-fixes. I wanted to fix all manually because during that process I checked which Just checked this right now: seems you're right and there is no auto-fix functionality. |
|
I'll prepare the first one of smaller PRs later today, so we'll be able to discuss specific decisions there. |
StrikerRUS
changed the title
We have a lot of yaml files across the repo, so I thinks it'll useful to lint them.
The diff of this PR looks quite large. I think it'll be better to split this PR into several smaller ones similarly to how we did with other linting PRs.
Opening this PR to get some initial comments from reviewers.