-
Notifications
You must be signed in to change notification settings - Fork 39
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
Upgrade Checkstyle integration test target 10.14.0 -> 10.21.1 #1425
Conversation
Looks good. No mutations were possible for these changes. |
@@ -366,6 +366,12 @@ | ||
<version>1.4.1</version> | ||
@@ -368,6 +368,12 @@ | ||
<version>1.4.4</version> |
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.
Hmm, this one is unexpected, if you don't recognize this one, I'll try to remove it @Stephan202 😄
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.
This is a change in the patch context, not a change in the changes made by the patch. So this is not unexpected 👍
/integration-test |
Looks good. No mutations were possible for these changes. |
Quality Gate passedIssues Measures |
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.
Suggested commit message:
Upgrade Checkstyle integration test target 10.14.0 -> 10.21.1 (#1425)
(This phrasing is a bit different from what we did before, but if we start to file this kind of PR more often, then the summary messages will be more informative.)
@@ -366,6 +366,12 @@ | ||
<version>1.4.1</version> | ||
@@ -368,6 +368,12 @@ | ||
<version>1.4.4</version> |
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.
This is a change in the patch context, not a change in the changes made by the patch. So this is not unexpected 👍
Instead of |
Well, the integration test targets a specific Checkstyle version. The specified tag is the test target. |
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 the summary, LGTM 🚀
I also prefer the alternative message 😄
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 meant to approve ✅
@Stephan202 Two questions;
|
Re 1: not opposed, but as a non-maintainer reading the release notes, I'm not sure I'd care much about the details of our testing setup. We could of course introduce a separate label and then map it to the same release notes section. If you think that helps with filtering: works for me. Re 2: we'd have to check whether this is compatible with GitHub actions, but perhaps it'd be even nicer if stuff "just worked", like this. The alternative is to parse |
Suggested commit message (similar to the previous one):
Or my slightly preferred alternative: