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

Running jgitver should be optional #58

Closed
McFoggy opened this issue Sep 11, 2017 · 8 comments
Closed

Running jgitver should be optional #58

McFoggy opened this issue Sep 11, 2017 · 8 comments

Comments

@McFoggy
Copy link
Contributor

McFoggy commented Sep 11, 2017

From @MattFriedman on September 9, 2017 3:22

If jgitver exists in the extensions.xml file, then it always runs, even if you are performing maven operations that don't require this versioning feature.

So, I hope you can implement a flag such as perhaps -Djgitver.versioning=true|false so that we can run the plugin or not depending on what is appropriate at the time.

Copied from original issue: jgitver/jgitver#30

@McFoggy
Copy link
Contributor Author

McFoggy commented Sep 11, 2017

@MattFriedman implementing such a flag is not difficult but I'd prefer that you explain why you would like it. Is that because of performances, a slow resolution of the version?

@MattFriedman
Copy link

Hi!

We have a peculiar case where we run maven-flyway as part of install and this runs jgitver as well. But in this particular case we have cloned a branch using some tricks to make the clone smaller, so that the cloned repository does not contain certain tags/commits etc... The missing git objects cause jgitver to throw exceptions. At the same time jgitver and versioning is not relevant for this task. So, we are getting errors from an unrelated coupled concern (versioning). It should IMO be easier for me to uncoupled the versioning concern when I would like to. ATM, I am deleting the extensions.xml file before running the above scenario so that jgitver will not be in play at all. (all of this happening in Jenkins)

It is generally my preference to have maven steps do as little as possible and then to explicitly include other concerns as needed. This creates the least amount of surprise for future maintainers, IMO. But that's just my approach.

@McFoggy
Copy link
Contributor Author

McFoggy commented Sep 11, 2017

It is generally my preference to have maven steps do as little as possible

I fully agree to that but jgitver goal/essence is to calculate a version. That's why I'd like not to "pollute" the code with such a flag.

If you do not have any other maven extension that would require you to keep the extensions.xml file then, as you are already doing, I'd suggest just to remove/rename the file to bypass jgitver.

Is that ok for you?

@MattFriedman
Copy link

No, because sometimes I want to run maven without running jgitver, and deleting the extensions.xml file in order to do this is a workaround/kludge. What happens when I end up with other useful extensions that I want to run and I need to delete it too? I don't see deleting the extensions.xml file as a valid way to solve this issue.

I wouldn't frame a flag for running/not running an extension as polluting it. It is simply a way to make your extension more flexible for users. Many other maven projects have such flags such as flyway and surefire.

Your plugin is really useful and I appreciate the work you have done, but you seem to be saying that users either have to run it always or never.

@McFoggy
Copy link
Contributor Author

McFoggy commented Sep 11, 2017

but you seem to be saying that users either have to run it always or never

Not at all, I just want to understand the usage. If you tell me that you have multiple maven extensions in use, it is enough for me to establish that at least someone has the need for.

@MattFriedman
Copy link

I appreciate it. Thanks and I look forward to using this further. It's a valuable extension.

@McFoggy
Copy link
Contributor Author

McFoggy commented Sep 13, 2017

@MattFriedman please use version 1.0.0 with -Djgitver.skip=true as explained in the README

@MattFriedman
Copy link

MattFriedman commented Sep 13, 2017 via email

@jhammell jhammell mentioned this issue Aug 3, 2018
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants