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

simplify pom by using simple property rather than xml fragment #7580

Merged
merged 3 commits into from
Jul 13, 2022

Conversation

olamy
Copy link
Member

@olamy olamy commented Feb 14, 2022

Signed-off-by: Olivier Lamy [email protected]

@olamy olamy added the Build label Feb 14, 2022
@@ -10,6 +10,11 @@
<artifactId>http3-tests</artifactId>
<name>Jetty :: HTTP3 :: Tests</name>

<properties>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbordet I assume we should not deploy those tests as part of the release and was a mistake? or I am wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module will only produce a test jar (no sources), so it won't be deployed anyway, right?
Ah, I see it deployed an empty jar 😒
So yes let's skip deploy for this module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after digging a bit the jar is not empty as it included some content we put in target/classes as part of the build such: generated MANIFEST.MF, LICENSE and NOTICE.txt

<configuration>
<!-- must not deploy -->
<skip>false</skip>
</configuration>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say I was confused here and not sure what to believe the comment or the code. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be the same as the other osgi test projects ;)

@@ -10,6 +10,11 @@
<artifactId>http3-tests</artifactId>
<name>Jetty :: HTTP3 :: Tests</name>

<properties>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module will only produce a test jar (no sources), so it won't be deployed anyway, right?
Ah, I see it deployed an empty jar 😒
So yes let's skip deploy for this module.

jetty-websocket/websocket-jetty-tests/pom.xml Show resolved Hide resolved
@olamy olamy marked this pull request as draft February 14, 2022 09:35
@olamy olamy marked this pull request as ready for review February 15, 2022 01:27
@olamy olamy requested a review from sbordet February 15, 2022 01:27
sbordet
sbordet previously approved these changes Feb 16, 2022
<configuration>
<!-- must not deploy -->
<skip>false</skip>
</configuration>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be the same as the other osgi test projects ;)

<artifactId>maven-deploy-plugin</artifactId>
<configuration>
<!-- No point deploying jmh project -->
<skip>true</skip>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs the property defined? Or is that covered by the toplevel tests/pom.xml properties?

Copy link
Member Author

@olamy olamy Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. properties are inherited.

sbordet
sbordet previously approved these changes Feb 25, 2022
@janbartel
Copy link
Contributor

@olamy this one needs merge conflicts fixed

Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes themselves look ok, but this branch needs re-merging from jetty-10.0.x.

@janbartel
Copy link
Contributor

@olamy nudge, needs merge.

@olamy
Copy link
Member Author

olamy commented Jul 13, 2022

@janbartel oh yes I definitely forgot this old PR. thanks for the ping :)

@olamy olamy force-pushed the jetty-10.0.x-simplify-poms branch from 1b703ae to f8dfdcd Compare July 13, 2022 07:59
@olamy olamy requested a review from janbartel July 13, 2022 10:10
@olamy olamy merged commit 9149945 into jetty-10.0.x Jul 13, 2022
@olamy olamy deleted the jetty-10.0.x-simplify-poms branch July 13, 2022 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants