-
Notifications
You must be signed in to change notification settings - Fork 88
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
#85 Consider ci-friendly-maven also for archetype #342
Conversation
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.
@sujith-mn Thanks for implementing this story. You properly solved this and I wonder why we havent been able to solve this earlier or what have been the problems in the first place. Anyhow it is great that you made it and we finally get this done 👍
Can you please fix the indendation (see our DoD) so I can approve and merge?
I also added some nice to have imporvement that you can have a look at. But that can be done after merging this PR or never if not easily doable.
<fileSet packaged="false" filtered="true" encoding="UTF-8"> | ||
<directory>.mvn</directory> | ||
<includes> | ||
<include>**/*.*</include> | ||
</includes> | ||
</fileSet> |
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.
Can you fix the indendation (here, and in all POMs you changed)... thanks.
@@ -19,6 +19,7 @@ | |||
<jackson.version>$[jackson.version]</jackson.version> <!-- Overriding Jackson for fixing vulnerabilities --> | |||
<guava.version>$[guava.version]</guava.version> | |||
<devonfw.test.excluded.groups>system</devonfw.test.excluded.groups> | |||
<app.version>${revision}${changelist}</app.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.
Excellent, this is exactly what we wanted to archive.
Please fix indendation and we can merge.
@@ -0,0 +1 @@ | |||
-Drevision=0.0.1 -Dchangelist=-SNAPSHOT |
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.
An archetype can take a version as argument. That is available via ${version}
variable.
It will now be ignored and the initial version is hardcoded to this 0.0.1-SNAPSHOT
.
As ${version}
can also be 1.0.0-SNAPSHOT
we can not easily do
-Drevision=${version} -Dchangelist=-SNAPSHOT
As this would then result in 1.0.0-SNAPSHOT-SNAPSHOT
.
However, as a nice to have, we could check if via velocity macros it is possible to strip out a potential -SNAPSHOT
from ${version}
. If that is possible, we should use ${version}
to make it perfect.
However, for me also fine to approve and merge like 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.
The formatting is done..
version passing as an argument would be really nice..
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 formatting. To prevent blocking this PR, I will merge it now.
Can you please create a new issue for the nice-to-have improvement to put the version here and research velocity macro syntax to remove potential -SNAPSHOT
suffix?
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.
Issue #346 created.
is it okay to directly place the version to app.version properties ... like below
#if($version)
<app.version>${version}</app.version>
#else
<app.version>${revision}${changelist}</app.version>
#end
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. Ready to merge now.
#if ($earProjectName != '.') | ||
#if ($earProjectName != '.') | ||
<module>${earProjectName}</module> | ||
#end | ||
#if ($batch == 'batch') | ||
#end | ||
#if ($batch == 'batch') | ||
<module>batch</module> | ||
#end | ||
#end |
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.
Seems Eclipse formatter also indents velocity macros.
We need to check the generated output as this might break the indentation there.
I hoped eclipse would leave things like this as is...
If this is a problem in the resulting pom we can fix in the new story for improvement.
No description provided.