-
Notifications
You must be signed in to change notification settings - Fork 419
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
Implement dot option #316
Implement dot option #316
Conversation
- path/to/folder/**/* | ||
- path/to/folder/**/.* |
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.
What about path/to/folder/sub/.folder/hello.txt
. Will it match? 🤔 If not, what would be the right set of globs?
README.md
Outdated
@@ -116,7 +120,23 @@ Various inputs are defined in [`action.yml`](action.yml) to let you configure th | |||
| `repo-token` | Token to use to authorize label changes. Typically the GITHUB_TOKEN secret, with `contents:read` and `pull-requests:write` access | N/A | | |||
| `configuration-path` | The path to the label configuration file | `.github/labeler.yml` | | |||
| `sync-labels` | Whether or not to remove labels when matching files are reverted or no longer changed by the PR | `false` | |||
| `dot` | Whether or not to auto-include paths starting with dot (e.g. `.github`) | `false` |
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.
WDYT about dropping the configuration option and making dot
always true.
If someone really wants to ignore dot files, that is another glob expression, right? (Something already supported.) IMO, if we can solve the same problem with less configuration it will be more user friendly and more likely to "do what I mean".
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.
That would be a breaking change which could upset existing users. It’s safer to introduce the option, then wait for a few months, change the default in a major release, wait again and finally remove the option completely.
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.
Yes, I agree it changes the behavior, but perhaps it is a reasonable one. We could also bump the major version to play it safe with expectations. Ultimately, it is not my decision. It is only a suggestion as an end user.
+1 Thanks for fixing this, and it would be great if this was merged in. I work at Datadog and we're using this in our repositories (see DataDog/dd-trace-rb#2291). |
@@ -57,7 +57,7 @@ From a boolean logic perspective, top-level match objects are `OR`-ed together a | |||
```yml | |||
# Add 'label1' to any changes within 'example' folder or any subfolders | |||
label1: | |||
- example/**/* | |||
- example/** |
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.
hi @kachkaev, we will take a look at this PR :) |
## 🌟 What is the purpose of this PR? The aim of this PR is to re-enable Slack notification for changes related to HASH app. It replaces a few globs for `packages/hash/...` with `apps/hash-*/**`, `libs/hash-*/**` and `libs/@local/hash-*/**`. To avoid repetition, I replaced the official labeler versio with the latest commit from actions/labeler#316. <!-- Explain, at a high level, what this does and why. --> <!-- Use the 'What does this change?' section to list more specific implementation details. --> ## 🔗 Related links <!-- Add links to any context it is worth capturing (e.g. Issues, Discussions, Discord, Asana) --> <!-- Mark any links which are not publicly accessible as _(internal)_ --> <!-- Don't rely on links to explain the PR, especially internal ones: use the sections above --> - [Asana task](https://app.asana.com/0/1203543026679373/1203800233887959/f) _(internal)_ - [Slack thread](https://hashintel.slack.com/archives/C02THT2LG8N/p1674566469357439) _(internal)_ ## 🐾 Next steps - Check if labeler works (we can only see this once the PR is merged because we use `pull_request_target` as a trigger) - Wait for `dot` option to reach GA and use the official labeler. - We can also consider syncing the labels with BP repo and get rid of `A/B/C-`. This does not bring a lot of added value in short term, so I haven’t tasked that out. ## 🛡 What tests cover this? - Linting - CI in actions/labeler#316
What else are we missing before the PR can be merged? 🙏 |
I'm glad that this has finally been merged. Thank you for everyone's efforts! 🙂 |
v4.1.0 is out with this new option 🎉 If anyone has been using my fork, please switch back to the official labeler action: - - uses: kachkaev/labeler@012b89238e3fa57d7af8ee028f02be6d421f184f
- ## @todo replace with actions/labeler@v4 (or newer) when this PR is merged:
- ## https://github.com/actions/labeler/pull/316
+ - uses: actions/labeler@v4
with:
dot: true My fork will not receive any updates and I plan to delete it in a couple of months. This means that workflows using I suggest to make |
After new release of the labeler action, changes to dot files are now tracked. This is available since v4.1. As we're tracking v4, it's going to be automatically picked up and we can make use of the new option. References actions/labeler#316 Fixes solidusio#4804
References actions/labeler#316 Fixes solidusio#4804
References actions/labeler#316 Fixes #4804 (cherry picked from commit edfc321)
References actions/labeler#316 Fixes #4804 (cherry picked from commit edfc321)
References actions/labeler#316 Fixes #4804 (cherry picked from commit edfc321)
References actions/labeler#316 Fixes #4804 (cherry picked from commit edfc321)
@kachkaev thanks for your time and contribution! |
The |
Per actions/labeler#316 (comment) the `dot: true` option in `.github/workflows/labeler.yml` can be removed, as `dot` is now set to `true` by default in v5.0.0
Closes #135