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(labeler): Fix RFC exclusion from docs, adjust other globs #3360

Merged
merged 3 commits into from
Jan 14, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 14, 2024

A little tune up in three parts:

  1. I noticed in my fork that the docs label was added to every PR. I went and reread very very attentively the docs for the labeler action, and noticed I missed that in the example for exclusion, there was a top level key that wasn’t any but all. So, inside the changed-files, the results of the options any-glob-to-any-file, any-glob-to-all-files, all-globs-to-any-file and all-globs-to-all-files were OR-ed together. That is because by default, if any or all is missing, any is used. I expected the results of the individual sections of globs to be AND-ed together, ie the behavior of all.
  2. Since I reread so attentively the docs, I observed that blabla/**/* was redundant to blabla/**, as a single star matches everything except a slash, and a double star matches everything. Their examples also show it.
  3. In the recent PRs of @ninsbl , I noticed that database wasn’t included as a label, and was in fact removed automatically even if it was added manually before the run of the labeller. It isn’t a surprise, as was explained as a potential shortcoming that I considered tolerable and isn’t enabled by default. So I added sql files to the database label.

For both points 1 and 2, I « changed » my main branch with the changes one by one and I did PRs in my fork to be able to test that the behaviour is correct. (The trigger is at pull_request_target, so the workflow already needs to be in the target to run)

@github-actions github-actions bot added CI Continuous integration docs labels Jan 14, 2024
@neteler neteler added this to the 8.4.0 milestone Jan 14, 2024
@echoix
Copy link
Member Author

echoix commented Jan 14, 2024

@neteler Would you mind taking a quick look at this? It solves the misconfiguration that ends up adding the docs label to every PR

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Looks good to me!

(The blabla/**/* pattern was given as example at https://github.com/actions/labeler at the time I initially created original PR, indeed redundant.)

Copy link
Member

@neteler neteler left a comment

Choose a reason for hiding this comment

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

I can't really judge the any-glob-to-any-file filter but it looks reasonable.

@echoix
Copy link
Member Author

echoix commented Jan 14, 2024

Looks good to me!

(The blabla/**/* pattern was given as example at https://github.com/actions/labeler at the time I initially created original PR, indeed redundant.)

The v5 in december changed a lot of the config. And the place they use it is not enough to know if it is good everywhere or for this advanced case only. I suppose the writer used this (for that example) as it is similar to an edge case in the cache/artifact upload with exclusions. In these, if you have a glob that includes files and subdirectories, but you want to exclude a specific subdirectory, the inclusion and exclusion pattern must have the same levels of hierarchy for the exclusion to work correctly. It is related to the way they squash down the directories for compression, such as there isn't 10 folders with a common path before the items start getting different. But here, how it's written, they seem independent and the result of the globs are used for the boolean logic.

@echoix echoix merged commit ddf374e into OSGeo:main Jan 14, 2024
24 checks passed
@echoix echoix deleted the labeler-exclusions-and-glob branch January 14, 2024 16:48
echoix added a commit to echoix/grass that referenced this pull request Jan 14, 2024
* CI(labeler): Fix RFC exclusion from docs, adjust other globs (OSGeo#3360)

* CI(labeler): Fix RFC exclusion from docs

* CI(labeler): Simplify double star globs when appropriate

* CI(labeler): Add database to `**.sql` files

* CI: Use multiple cores for grass build in CodeQL

* CI: Add concurrency group for CodeQL

* CI(CodeQL): Run all analysis concurrently, but don't build for python

* CI(CodeQL): Remove Python dependency installation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants