-
Notifications
You must be signed in to change notification settings - Fork 299
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
[AMORO-3152] Make git-commit-plugin fail on no git dir configurable #3126
Conversation
@zhoujinsong @zhongqishang Could you please have a look at this when you're free, thanks. Could you please share where the release script is located in |
You can add this new profile here: https://github.com/apache/amoro/blob/master/tools/releasing/create_binary_release.sh#L68 |
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.
@klion26 Thanks for the contribution, I left some comments.
pom.xml
Outdated
<groupId>pl.project13.maven</groupId> | ||
<artifactId>git-commit-id-plugin</artifactId> | ||
<version>4.0.0</version> | ||
<configuration> |
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 am not sure if the profile plugin configuration will overwrite the default configuration or merge into the default configuration.
As you just added the new configurations, it may seem to be they will be merged into the default configuration, do we have any documentation about profile behavior about this?
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.
sorry for not attaching the result tested in my local env, has been attached in the end now
the configuration in the profile will overwrite these in the build configuration. the priority for the configuration is: configuration in profile(after enabled)
> build configuration
. can ref the resource[1][2] here.
the result after the profile enabled
the result for default(not enable the profile)
[1] https://maven.apache.org/guides/introduction/introduction-to-profiles.html?spm=5176.28103460.0.0.6077572cRnkc6p#profile-inheritance
[2] https://maven.apache.org/guides/introduction/introduction-to-the-pom.html#Project_Inheritance
80dec7c
to
8fd9cc7
Compare
After an offline discussion with @zhongqishang we reached to consense that: add a new profile to enable the The code has been changed to the above change, please have a look when you're free thanks. @zhoujinsong @zhongqishang normal build build after the check enabled |
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.
IMO, not failing by default is a good choice. This PR also provides the option to fail.
LGTM.
53942a2
to
96bc056
Compare
We add conf to fail silent when no git dir before, so that it would not block the release process, but it may not fail in normal mode also, in this commit we add a profile to enable the check, so that build will fail when no git dir found.
96bc056
to
9710f63
Compare
The pr changed to using a profile with configuration inspired by @zhoujinsong The result can be found below |
Thanks for the contribution! @klion26 |
@zhoujinsong @zhongqishang thanks for the review and merging! |
Why are the changes needed?
We add conf to fail silent when no git dir before, so that it would not block the
release process, but it may not fail in normal mode also, in this commit
we move the configuration to profile and can be enabled in the release process
Close #3125 .
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation