-
Notifications
You must be signed in to change notification settings - Fork 375
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
Action:Automatic pull request labeler #2291
Conversation
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.
👍 Thanks for being our GitHub wizard and pushing our automation forward :)
.github/labeler.yml
Outdated
# Version bump pull request | ||
release: | ||
- all: [ '{CHANGELOG.md,lib/ddtrace/version.rb,gemfiles/**}' ] |
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.
Really minor: Since we've done a few releases where we didn't bump all the gemfiles, I wonder if it would make sense to relax this rule to take that into account.
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.
Is that so? When a release is done at least ddtrace (x.y.z)
should get an update in the lockfiles, correct?
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 and no. The way we've set up CI is that the CI runners will do the update locally anyway, thus we can actually not update it and things will still be fine.
But we do want the lockfiles to be updated from time to time, so this is why we have it as a step of the release process.
So my question/suggestion was more of a: should we consider here the exceptional cases where we do a release and skip the lockfile update?
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.
I'm removing gemfile bump from this list, to ensure we have no false positives on the release
tag.
.github/labeler.yml
Outdated
|
||
# Changes to Tracing integrations | ||
integrations: | ||
- any: [ 'lib/datadog/tracing/contrib/**' ] |
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.
Should there be 'lib/datadog/appsec/contrib/**'
here too? Maybe `'lib/datadog/ci/contrib/**' as well?
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.
Also should there be a general tracing one as well?
tracing:
- any: [ 'lib/datadog/tracing/**' ]
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.
Not sure what the behaviour is when multiple criteria apply (e.g resulting in AppSec
+Integrations
tags, which sounds good to me if someone changes AppSec integration code).
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.
Also this non-product one sounds OK to me as well:
core:
- any: [ 'lib/datadog/core/**' ]
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.
Applied all 3 suggested changes.
|
||
# Changes to CI-App | ||
ci-app: | ||
- any: [ 'lib/datadog/ci/**' ] |
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.
Thanks for giving my fork a go @marcotc! Does this glob work the same as lib/datadog/ci/**/*
? I can update docs in actions/labeler#316 if so.
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.
Thank you for the changes, @kachkaev! Couldn't use the GH Action without it! 🙇
Yes, it works the same.
minimatch("root/a/b/c", "root/**/*")
// true
minimatch("root/a/b/c", "root/**")
// true
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
This PR automatically adds GitHub labels to PRs where it's safe to assume their intent.
Some examples are PRs that: