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

[WFLY-18289] Correct GAV handling #17043

Merged
merged 1 commit into from
Aug 3, 2023
Merged

Conversation

bstansberry
Copy link
Contributor

Fix incorrect use of maven properties for GAVs
Consolidate on a single scheme for controlling testsuite feature pack GAVs Use the wildfly-standard-expansion-bom in testsuite/layers-expansion

https://issues.redhat.com/browse/WFLY-18289

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 24, 2023
Fix incorrect use of maven properties for GAVs
Consolidate on a single scheme for controlling testsuite feature pack GAVs
Use the wildfly-standard-expansion-bom in testsuite/layers-expansion
Copy link
Contributor

@yersan yersan left a comment

Choose a reason for hiding this comment

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

Wouldn't be possible to get rid of ee.maven.groupId? From 10000 foot view, we only want to split the GA in two namespaces, however, we are right now using three properties for that project.groupId, ee.maven.groupId, and full.maven.groupId.

So I'm just wondering if makes sense and if it is possible to reduce them to just two project.groupId and full.maven.groupId. In general, these properties tend to get out of sync easily on the PR reviews, so thinking only when full.maven.groupId is required in a new PR makes this sync more stable.

For sure I understand that would require an additional step before creating a new project that will only use the full.maven.groupId, but sounds loadable since it would be just changing the existing project.groupId to ee.maven.groupId before creating the branch for that new project.

@bstansberry
Copy link
Contributor Author

@yersan Perhaps get rid of project.groupId? It is inherently ambiguous, and if it's gone but we see it re-added in code review we can require its replacement by the author. If we allow it then reviewers have to analyze every new use for correctness.

Both project.groupId are heavily used, so in both cases a bulk change would be required. Of course, project.groupId is more heavily used, but sed or whatever would do the bulk change won't care.

@bstansberry
Copy link
Contributor Author

Note that getting rid of ee.maven.groupId won't work downstream.

@yersan
Copy link
Contributor

yersan commented Aug 1, 2023

Thanks @bstansberry , as discussed, my comment requests more changes outside of the main scope of this PR and we do not want to delay it due to those comments, so this should be good.

Bootable JAR failure is unrelated. And cloud tests should not be affected either since this PR is not changing any GA. For upstream all ee.maven.groupId and full.maven.groupId and project.groupId are all the same.

@bstansberry
Copy link
Contributor Author

Thanks @yersan. We should definitely continue thinking about your suggestion.

@pferraro
Copy link
Contributor

pferraro commented Aug 1, 2023

@yersan Perhaps get rid of project.groupId? It is inherently ambiguous, and if it's gone but we see it re-added in code review we can require its replacement by the author. If we allow it then reviewers have to analyze every new use for correctness.

Both project.groupId are heavily used, so in both cases a bulk change would be required. Of course, project.groupId is more heavily used, but sed or whatever would do the bulk change won't care.

Why is project.groupId inherently ambiguous? By definition, it resolves to the groupId of the pom in which it is used.
IMO, this expression is perfectly appropriate for intra-project dependencies.

@bstansberry
Copy link
Contributor Author

bstansberry commented Aug 1, 2023

Why is project.groupId inherently ambiguous? By definition, it resolves to the groupId of the pom in which it is used. IMO, this expression is perfectly appropriate for intra-project dependencies.

Because this system of groupId expressions isn't intended for the intra-project dependency use case. It exists to facilitate forking the WildFly code base into two separate downstream projects that use different groupIds. So when you look at project.groupId in terms of how it will impact a WildFly build, it's unambiguous. But if you look at how it will impact a downstream build, it's "ambiguous". "Ambiguous" isn't really the correct word, but something like that. :)

Most of the changes in this PR are a forward port of things I needed to correct to get the EAP XP 5 branch to build.

@bstansberry bstansberry merged commit 9d48cbd into wildfly:main Aug 3, 2023
13 of 14 checks passed
@bstansberry bstansberry deleted the WFLY-18289 branch August 3, 2023 21:19
@bstansberry
Copy link
Contributor Author

Thanks @yersan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
3 participants