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

Updating quarkus examples pom.xml files #1988

Closed

Conversation

LightGuard
Copy link

Updating the pom files to remove properties which make it confusing for end users. This removes most properties in the pom. The properties removed are only there the change the gav for some dependencies.

Removing unnecessary properties and substituting what should be there.
This cleans up the pom files and removes potentially confusing lines in
the pom.xml files for examples.

Signed-off-by: Jason Porter <[email protected]>
@jomarko
Copy link

jomarko commented Aug 5, 2024

I will review this tomorrow and check the relationship for the exiting PR #1984. That, probably will be closed, need to check.

<version>${quarkus.platform.version}</version>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-bom</artifactId>
<version>3.8.4</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it better to keep versions defined as part of properties instead of moving them to the dependency block?
I agree that some properties you removed make sense to be removed to make it simple, but maybe keep the versions in the properties block?

Copy link
Contributor

@fjtirado fjtirado Aug 8, 2024

Choose a reason for hiding this comment

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

In my opinion, version (Kogito one, which is used twice in the same file and even quarkus one, which is used once, that way the two will be together) should be kept in properties block. It will be easier to change.

@mareknovotny
Copy link
Contributor

hard-coding of version is always not good as that requires manual changes on multiple places later and regularly.

@LightGuard
Copy link
Author

I fully understand the argument, and I thought about it. I typically do just that as well. However, when I was doing this one, I looked hard at:

  1. how many places do we have this version? Answer: One place. Does it matter if it's in a version tag or a property tag? It is still one place we have to change it.
  2. these are examples for people using our software, probably for the first time. They may not be familiar with Maven (I know that sounds odd, but it is true). The easier we can make these examples to follow and use for future starting places for users, the more I think it will work out to our advantage.

@fjtirado fjtirado closed this Aug 8, 2024
@LightGuard
Copy link
Author

Why was the PR closed?

@fjtirado fjtirado reopened this Aug 8, 2024
@fjtirado
Copy link
Contributor

fjtirado commented Aug 8, 2024

Why was the PR closed?

A home accident, reopened. Apologies

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

As stated in the mailing list by @gitgabrio, removing parent and version properties, even for examples, is not the best approach. The Java community has been using this approach for years, and as exemplified in the list, Spring Framework (the most popular Java framework out there) also uses it.

Copy link
Contributor

@gitgabrio gitgabrio left a comment

Choose a reason for hiding this comment

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

Thanks @LightGuard , but as I stated elsewhere, IMO this refactoring goes exactly other way respect maintainability

@LightGuard
Copy link
Author

I'll close this one and create a new PR.

@LightGuard LightGuard closed this Aug 8, 2024
@LightGuard LightGuard deleted the update-pom-remove-properties branch August 8, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants