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

Use metadata for gradle/maven repositories and separate gradle plugin version #11296

Merged
merged 1 commit into from
Aug 19, 2020
Merged

Use metadata for gradle/maven repositories and separate gradle plugin version #11296

merged 1 commit into from
Aug 19, 2020

Conversation

ia3andy
Copy link
Contributor

@ia3andy ia3andy commented Aug 10, 2020 •
edited
Loading

As part of #11256

This PR:

  • use the platform metadata field in order to define repositories and plugin repositories for the different build tools (in legacy codegen and codestarts)
  • fix a bug with the gradle wrapper for kotlin dsl in codestarts (wrong directory)
  • separate maven plugin version from gradle plugin version as they are independent (even if the same for now) and make it customizable at platform level
  • make quarkus.properties props naming more intuitive because plugin-version was often misused in the code

@boring-cyborg boring-cyborg bot added the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Aug 10, 2020
@ia3andy ia3andy requested a review from maxandersen August 10, 2020 09:38
@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 13, 2020

@maxandersen I think we can merge this anyway?

@maxandersen
Copy link
Member

I would remove the docker changes. Not sure I see a usecase for them.

We need to add in this or other passing of maven parameters to the old cidegen.

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 13, 2020

@maxandersen

I would remove the docker changes. Not sure I see a usecase for them.

It's better to have them as data in one place (easier to maintain)

We need to add in this or other passing of maven parameters to the old cidegen.

I will do that in another PR

@ia3andy ia3andy changed the title Parametrize Codestarts docker containers and gradle/maven repositories (wip) Parametrize Codestarts docker containers and gradle/maven repositories Aug 13, 2020
@ia3andy ia3andy changed the title (wip) Parametrize Codestarts docker containers and gradle/maven repositories (wip) Use metadata for gradle/maven repositories and separate gradle plugin version Aug 17, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 17, 2020 •
edited
Loading

It should be alright, but I am not yet sure I applied the change everywhare, waiting for my fork CI tests to pass.

I will also need to take some stuff from this to put in 1.7.X (as a separate dedicated lighter PR)

@ia3andy ia3andy closed this Aug 17, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 17, 2020

snap I closed it some way with my keyboard :-/

@ia3andy ia3andy reopened this Aug 17, 2020
@ia3andy ia3andy force-pushed the codestarts-up branch 2 times, most recently from ecec626 to 7f96623 Compare August 18, 2020 08:12
@ia3andy ia3andy changed the title (wip) Use metadata for gradle/maven repositories and separate gradle plugin version Use metadata for gradle/maven repositories and separate gradle plugin version Aug 18, 2020
@ia3andy ia3andy added this to the 1.8.0 - master milestone Aug 18, 2020
@ia3andy ia3andy mentioned this pull request Aug 18, 2020
Merged
@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 18, 2020

@maxandersen @aloubyansky @gastaldi this PR is ready to be reviewed

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 18, 2020

@maxandersen @gsmet @mgorniew fyi this PR is pretty important/urgent as it also fixes the Gradle wrapper that was in the wrong directory (for Gradle Kotlin DSL in codestarts only)..

aloubyansky
aloubyansky reviewed Aug 19, 2020
View reviewed changes
devtools/platform-descriptor-json/src/main/filtered/quarkus.properties
# Plugin metadata
plugin-groupId=${project.groupId}
plugin-artifactId=quarkus-maven-plugin
plugin-version=${project.version}
Copy link
Member

Choose a reason for hiding this comment

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

Removing plugin-version will break QuarkusJsonPlatformDescriptorResolver.getBundledPlatformQuarkusVersionOrNull()

Copy link
Member

Choose a reason for hiding this comment

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

Is this targeting 1.8.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is targeting 1.8.x (should be alright right?). This is why I made a different PR for 1.7.x #11439

Copy link
Member

Choose a reason for hiding this comment

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

wait - why are we talking about removing plugin-version ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there was too much confusion in the code with it (it was used as quarkus-core-version), it is now split in 3:

quarkus-core-version=${project.version}
...
maven-plugin-version=${project.version}
...
gradle-plugin-version=${project.version}

maxandersen
maxandersen reviewed Aug 19, 2020
View reviewed changes
...java/io/quarkus/platform/descriptor/resolver/json/QuarkusJsonPlatformDescriptorResolver.java
@@ -711,12 +711,7 @@ private static String getBundledPlatformQuarkusVersionOrNull() {
} catch (IOException e) {
throw new IllegalStateException("Failed to load quarkus.properties from the classpath", e);
}
final String quarkusVersion = props.getProperty("plugin-version");
Copy link
Member

Choose a reason for hiding this comment

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

hmm - isn't there cases where plugin-version is the only thing availalbe as a fallback?

Copy link
Contributor Author

@ia3andy ia3andy Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It would only fail with an older version of the platform I believe. But we don't have to do backward compat on new major, right @aloubyansky?

IMHO it's better to be not compatible here, than failing somewhere else because we missed something.

Copy link
Member

Choose a reason for hiding this comment

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

you aren't guaranteed to have a platform - even in newer versions.

Copy link
Member

Choose a reason for hiding this comment

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

misspoke - or rather, users can make a pom that has no platform ref; but that is not reelvant for here as this is during fully controlled codegenaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems you are mixing with something else right?

maxandersen
maxandersen requested changes Aug 19, 2020
View reviewed changes
Copy link
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

generally looks ok - just a few comment/questions to clarify.

@maxandersen maxandersen self-requested a review August 19, 2020 12:14
maxandersen
maxandersen approved these changes Aug 19, 2020
View reviewed changes
Copy link
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

lgtm

@ia3andy ia3andy merged commit 89f0ba4 into quarkusio:master Aug 19, 2020
@ia3andy ia3andy deleted the codestarts-up branch August 19, 2020 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgorniew mgorniew mgorniew approved these changes

@aloubyansky aloubyansky aloubyansky approved these changes

@maxandersen maxandersen maxandersen approved these changes

@gastaldi gastaldi Awaiting requested review from gastaldi

Assignees
No one assigned
Labels
area/codestarts area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins
Projects
None yet
Milestone
1.8.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@ia3andy @maxandersen @aloubyansky @mgorniew