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.

Sign up for GitHub

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

Jump to bottom

[SHRINKRES-316] Clean up the poms #180

Merged
merged 2 commits into from
Feb 28, 2024
Merged

[SHRINKRES-316] Clean up the poms #180

merged 2 commits into from
Feb 28, 2024

Conversation

petrberan
Copy link
Member

@petrberan petrberan requested a review from xstefank November 10, 2023 16:09
xstefank
xstefank approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

There is an enormous duplication. Maybe in future we could generate these files.

gradle/impl-gradle-embedded-archive/pom.xml
@@ -46,7 +46,7 @@
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-javac</artifactId>
<version>${version.org.codehaus.plexus.compiler.javac}</version>
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to keep it version.package.of.dependency?

pom.xml Outdated
<scope>import</scope>
<type>pom</type>
</dependency>
<dependency>
<groupId>org.jboss.shrinkwrap</groupId>
<artifactId>shrinkwrap-bom</artifactId>
<version>${version.org.jboss.shrinkwrap}</version>
Copy link
Member

Choose a reason for hiding this comment

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

probably keep like this

@petrberan petrberan mentioned this pull request Nov 22, 2023
Closed
@petrberan
Copy link
Member Author

Wrong commit, will resolve tomorrow

@petrberan
Copy link
Member Author

Fixed

xstefank
xstefank requested changes Nov 24, 2023
View reviewed changes
Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

I find it hard to believe you need to have a separate version for every artifact. Align what you can.

gradle/impl-gradle-embedded-archive/pom.xml
@@ -27,7 +27,7 @@
<dependency>
<groupId>org.gradle</groupId>
<artifactId>gradle-tooling-api</artifactId>
<version>${version.gradle-tooling-api}</version>
<version>${version.org.gradle.gradle-tooling-api}</version>
Copy link
Member

Choose a reason for hiding this comment

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

I still think you should go with package, not package.artifact. If we will have some automation in the future we should align this with what you will have in main pom.xml

Copy link
Member Author

Choose a reason for hiding this comment

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

Having problem understanding what is the desired outcome for this comment. Can you provide expected version property for - for example - arquillian-spacelift-api?

pom.xml
@@ -318,6 +331,7 @@
<format>html</format>
<format>xml</format>
</formats>
<check/>
Copy link
Member

Choose a reason for hiding this comment

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

what is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Check tag is required by the plugin config, in this case it doesn't do anything. This tag makes IDEA not consider it an error anymore

pom.xml
<version.org.codehaus.plexus.plexus-classworlds>2.7.0</version.org.codehaus.plexus.plexus-classworlds>
<version.org.codehaus.plexus.plexus-compiler-javac>2.7</version.org.codehaus.plexus.plexus-compiler-javac>
<version.org.codehaus.plexus.plexus-component-annotations>2.1.0</version.org.codehaus.plexus.plexus-component-annotations>
<version.org.codehaus.plexus.plexus-sec-dispatcher>2.0</version.org.codehaus.plexus.plexus-sec-dispatcher>
Copy link
Member

Choose a reason for hiding this comment

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

Try to align versions where you can from the same projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal of this issue is to clean up the poms, updating versions will be part of the future development

xstefank
xstefank approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

After @petrberan explained to me that he plans to finish remaining comments in a followup issue. I'm merging this.

@petrberan petrberan merged commit c7360e5 into shrinkwrap:master Feb 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xstefank xstefank xstefank approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@petrberan @xstefank