Skip to content
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

Merged
merged 2 commits into from
Aug 25, 2019
Merged

Allow to override useNativeGit from command line #433

merged 2 commits into from
Aug 25, 2019

Conversation

alexott
Copy link
Contributor

@alexott alexott commented Aug 18, 2019

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.

Contributor Checklist

  • Added relevant integration or unit tests to verify the changes
  • Update the Readme or any other documentation (including relevant Javadoc)
  • Ensured that tests pass locally: mvn clean package
  • Ensured that the code meets the current checkstyle coding style definition: mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B

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")
Copy link
Collaborator

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'...

Copy link
Contributor Author

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.

@TheSnoozer
Copy link
Collaborator

Hello,
thank you for your contribution.

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 mvn clean package and not be forced to think about the multiple command line options I need to fiddle with to get it to work).

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.

@alexott
Copy link
Contributor Author

alexott commented Aug 20, 2019

@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.

@TheSnoozer
Copy link
Collaborator

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 mvn clean package -DgitUseNativeGit=true)

@alexott
Copy link
Contributor Author

alexott commented Aug 20, 2019

Yes, this approach works just fine. I've submitted PR already: apache/zeppelin#3427

@TheSnoozer
Copy link
Collaborator

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....

@alexott
Copy link
Contributor Author

alexott commented Aug 20, 2019

I'll try to address it, but maybe only around end of the week...

@TheSnoozer
Copy link
Collaborator

Thanks! LGTM.

@TheSnoozer TheSnoozer merged commit 94a3b69 into git-commit-id:master Aug 25, 2019
@TheSnoozer TheSnoozer added this to the 3.0.2 milestone Aug 25, 2019
@alexott
Copy link
Contributor Author

alexott commented Aug 25, 2019

Thank you!

@alexott alexott deleted the use-native-git-command-line branch August 25, 2019 18:34
TheSnoozer pushed a commit to TheSnoozer/git-commit-id-maven-plugin that referenced this pull request Aug 25, 2019
@TheSnoozer
Copy link
Collaborator

You did the work so you deserve the credit :-)
I just clicked a button ;-)

@TheSnoozer TheSnoozer modified the milestones: 3.0.2, 4.0-modularized Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants