-
Notifications
You must be signed in to change notification settings - Fork 302
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
Allow to override useNativeGit from command line #433
Allow to override useNativeGit from command line #433
Conversation
Right now, because JGit doesn't support Git worktrees, I need to override `useNativeGit` in the `pom.xml` of different projects, and it's easy to make a mistake and commit the changes. The proposed PR should allow to override this property from command-line via `maven.gitcommitid.nativegit`, without modification of the `pom.xml`.
* @since 2.1.9 | ||
*/ | ||
@Parameter(defaultValue = "false") | ||
@Parameter(property = "maven.gitcommitid.nativegit", defaultValue = "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.
Unfortunately I don't think this is working with maven and I'm still convinced that this is still utterly broken in maven.
The NOTE/WARNING you have introduced (copy and pasted from one of the other setting?) refers to #315 where a similar problem was outlined for the skip
property.
Essentially the problem here is that maven somehow does not respect the command-line option and will use the option configured in the pom (if available). However whenever a user has configured the setting in the pom the command-line option should overwrite that pom setting (IMHO that is at least how maven behaves). AFAIK this is not happening and is only possible when we need to introduce an additional property.
This could look like something like this:
/** Set this to {@code 'true'} to use native Git executable to fetch information about the repository.
* It is in most cases faster but requires a git executable to be installed in system.
* By default the plugin will use jGit implementation as a source of information about the repository.
*
* @since 2.1.9
*/
@Parameter(defaultValue = "false")
boolean useNativeGit;
/**
* Set this to {@code 'true'} to use native Git executable to fetch information about the repository via commandline.
* NOTE / WARNING:
* Do *NOT* set this property inside the configuration of your plugin.
* Please read
* https://github.com/git-commit-id/maven-git-commit-id-plugin/issues/315
* to find out why.
* @since 3.0.2
*/
@Parameter(property = "maven.gitcommitid.nativegit", defaultValue = "false")
private boolean useNativeGitViaCommandLine;
Certainly whenever this plugin decided to run the native code it should run native whenever useNativeGitViaCommandLine
or useNativeGit
is true.
Why plugins are forced to do this this way is beyond me....and I would call it broken and not 'expected'...
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 advice - I've re-implemented it as you suggested. The new implementation checks, if the given property is set, and then use the value provided - this allows to overwrite setting even if useNativeGit
is explicitly set to true
in the project - for example, when I don't have Git installed on my machine.
Hello, I personally would argue that it is easier to configure the plugin inside the pom. A build in my opinion should not rely on command-line options set in the right way, but I guess that could be considered a personal preference (e.g. I would like to checkout a project and be able to run it via Have you considered using a parent pom that configures every plugin in the way/shape you need it and then have all your projects use that parent pom? Nevertheless I think this option should not hurt as an addition to this project/plugin. |
@TheSnoozer thank you for reviewing! I'm trying to make it configurable via properties, so I can override them if necessary... So maybe this PR won't be required. |
Sure think I'm more than happy to review! Let me know how your testing goes. Perhaps it would be easier to use a small project for testing purposes (e.g. I generally use https://github.com/TheSnoozer/git-commit-id-debugging/). In your case you could get away with using profiles or project properties (e.g. <properties>
<gitUseNativeGit>false</gitUseNativeGit>
</properties>
<plugins>
<plugin>
<groupId>pl.project13.maven</groupId>
<artifactId>git-commit-id-plugin</artifactId>
<version>${git-commit-id-version}</version>
<configuration>
<useNativeGit>${gitUseNativeGit}</useNativeGit>
</configuration>
</plugin>
</plugins> via |
Yes, this approach works just fine. I've submitted PR already: apache/zeppelin#3427 |
Interesting. If you use external projetcs that gather your git properties it would be a hassle to update them with the workaround. I guess if you address my comment this could be integrated into the plugin directly avoiding the need to have all consumers to update.... |
I'll try to address it, but maybe only around end of the week... |
Thanks! LGTM. |
Thank you! |
…n with the native implementation
You did the work so you deserve the credit :-) |
Right now, because JGit doesn't support Git worktrees, I need to override
useNativeGit
in the
pom.xml
of different projects, and it's easy to make a mistake and commit thechanges.
The proposed PR should allow to override this property from command-line via
maven.gitcommitid.nativegit
, without modification of thepom.xml
.Contributor Checklist
mvn clean package
checkstyle
coding style definition:mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B