Extracting common devtools from Quarkus core repo #4615

aloubyansky · 2019-10-16T15:41:07Z

ia3andy

LGTM

ia3andy · 2019-10-18T12:09:54Z

...java/io/quarkus/platform/descriptor/resolver/json/QuarkusJsonPlatformDescriptorResolver.java

+        return this;
+    }
+
+    public QuarkusPlatformDescriptor resolve() {


Maybe you could split that big method in a few sub private ones?

I would if those method would be re-usable in other places. Otherwise, it's not that long and I'm pretty sure it's be changing pretty soon anyway.

Sorry I am allergic to long methods 😂 I prefer shorter unit testable ones

ia3andy · 2019-10-18T12:18:25Z

devtools/maven/src/main/java/io/quarkus/maven/BuildFileMojoBase.java

+    @Override
+    public void execute() throws MojoExecutionException {
+
+        validateParameters();


@aloubyansky this one is too long too bug I think it's copied code right?

Not really, I actually type it in :)

ia3andy · 2019-10-18T12:19:43Z

devtools/maven/src/main/java/io/quarkus/maven/CreateUtils.java

+    public static void setupQuarkusJsonPlatformDescriptor(
+            RepositorySystem repoSystem, RepositorySystemSession repoSession, List<RemoteRepository> repos,
+            String platformGroupId, String platformArtifactId, String defaultPlatformVersion,
+            final Log log)


Why log is final but not the rest 😅

It doesn't have to be now. Originally, it was referenced from an anonymous class, so it had to be final.

ia3andy · 2019-10-18T12:22:01Z

...io/quarkus/platform/descriptor/loader/json/impl/QuarkusJsonPlatformDescriptorLoaderImpl.java

+
+    @Override
+    public QuarkusJsonPlatformDescriptor load(QuarkusJsonPlatformDescriptorLoaderContext context) {
+        context.getMessageWriter().debug("Loading Platform Descriptor from JSON %s", context.getJsonDescriptorFile());


too long too :)

This is not too long :)

it's too long :)

ia3andy · 2019-10-18T12:22:31Z

.../main/java/io/quarkus/platform/descriptor/loader/legacy/QuarkusLegacyPlatformDescriptor.java

+    private final MessageWriter log;
+
+    public QuarkusLegacyPlatformDescriptor(ClassLoader cl, MessageWriter log) {
+        this.cl = cl;


too much logic for a constructor, this might need a factory?

geoand · 2019-10-18T14:17:52Z

This is going to need a rebase for the CI fix.

Thanks

gastaldi · 2019-10-18T20:05:23Z

independent-projects/tools/pom.xml

+    <parent>
+        <groupId>org.jboss</groupId>
+        <artifactId>jboss-parent</artifactId>
+        <version>31</version>


Suggested change

<version>31</version>

<version>36</version>

maxandersen · 2019-10-21T09:10:24Z

just so I don't have to remember in future, context from https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/quarkus-dev/snbwLidlOq8/pbusfmiOAAAJ

"The main purpose of that PR is to detach the project generating and manipulating functionality from the manually maintained extensions.json file stored in the same JAR as the common devtools classes and also make the first step in the direction of making the tooling API not specific to a Quarkus release. Most of the code from devtools/common has been moved to independent-projects/tools for now.

The reason this was done now is for Quarkus 1.0 release we want to also release a so called Quarkus Platform that will include extensions that are in the Quarkus repo plus Camel Quarkus extension which has been developed and maintained in a different repo. The platform will have its own BOM and it's equivalent of the extensions.json file.
So to be able to promote the platform with the extended set of extensions through our tooling and code.quarkus.io, we need to be able to redirect the tools to a different metadata.

The PR introduces basic abstractions to do that, point to a specific release of the platform or look for the latest version of the desired "platform".

The extension.json file is now going to be generated (the previous manually maintained version is not removed, the tools can still be configured to use it as a legacy descriptor) from a BOM. There is a new Maven goal generate-extensions-json that generates a JSON file for a specific BOM by collecting metadata from the extensions it includes.
One such JSON file is now generated during the Quarkus build (devtools/bom-descriptor-json). So in Quarkus at this point we are going to have the legacy extensions.json file (which I kept for now, just in case) and the generated one (which is now the default one).

https://github.com/quarkusio/quarkus-platform will be releasing its own BOM and the JSON descriptor.
So the tools could be pointed to use either the platform metadata or the core Quarkus one.

It's not all perfectly crafted at this point, of course, it's just the first step.
"

maxandersen · 2019-10-21T09:17:30Z

...io/quarkus/platform/descriptor/loader/json/impl/QuarkusJsonPlatformDescriptorLoaderImpl.java

+                platform = mapper.readValue(is, QuarkusJsonPlatformDescriptor.class);
+            }
+        } catch (IOException e) {
+            e.printStackTrace();


should be removed.

maxandersen · 2019-10-21T09:20:26Z

independent-projects/tools/common/src/main/java/io/quarkus/maven/utilities/MojoUtils.java


    public static String getPluginArtifactId() {
-        return get("plugin-artifactId");
+        return "quarkus-maven-plugin";


is this really right? same question on the next 2 getters....why isn't this resolving the properties rather than being hardcoded ?

maxandersen · 2019-10-21T09:21:15Z

independent-projects/tools/common/src/main/java/io/quarkus/maven/utilities/MojoUtils.java

+    public static String getBomVersionForTemplate() {
+        final String v = getBomVersion();
+        if(v.equals(getQuarkusVersion())) {
+            // this might not always work but at this point we're assuming the bom is coming from Quarkus itself


this would (almost) always be wrong when using a platform wouldn't it ?

Yes, in which case we'll use specific version instead of ${quarkus.version}. I left it this way until we adjust the templates and the tests for the platform use-case.

gsmet · 2019-10-21T09:32:51Z

All the native tests are red AFAICS.

gsmet · 2019-10-21T18:54:47Z

Unfortunately, there's a conflict. I'm rebasing and running the tests. Will push soon.

gsmet · 2019-10-21T20:05:44Z

Rebased, force pushed and added a commit to exclude the sisu inject dependency from quarkus-core-deployment. Otherwise Kogito was generating a javax.inject.Named file again.

aloubyansky · 2019-10-21T20:06:30Z

ok, thanks @gsmet

aloubyansky force-pushed the platform-resolver branch 3 times, most recently from 7d2442d to 2ade2b0 Compare October 17, 2019 14:49

ia3andy self-requested a review October 17, 2019 15:04

aloubyansky force-pushed the platform-resolver branch from 2ade2b0 to dcaf27f Compare October 17, 2019 16:39

aloubyansky marked this pull request as ready for review October 17, 2019 16:51

aloubyansky added this to the 0.26.0 milestone Oct 17, 2019

aloubyansky added the area/platform Issues related to definition and interaction with Quarkus Platform label Oct 17, 2019

aloubyansky force-pushed the platform-resolver branch 2 times, most recently from f24f76e to c4842eb Compare October 18, 2019 08:51

ia3andy approved these changes Oct 18, 2019

View reviewed changes

ia3andy reviewed Oct 18, 2019

View reviewed changes

aloubyansky force-pushed the platform-resolver branch from c4842eb to b31a864 Compare October 18, 2019 15:09

gastaldi reviewed Oct 18, 2019

View reviewed changes

aloubyansky force-pushed the platform-resolver branch from b31a864 to 97eb496 Compare October 21, 2019 08:11

maxandersen reviewed Oct 21, 2019

View reviewed changes

aloubyansky force-pushed the platform-resolver branch from 97eb496 to 2381b27 Compare October 21, 2019 11:04

gsmet modified the milestones: 0.26.0, 0.27.0 Oct 21, 2019

gsmet added the backport? label Oct 21, 2019

aloubyansky force-pushed the platform-resolver branch from 2381b27 to e180646 Compare October 21, 2019 13:21

aloubyansky and others added 2 commits October 21, 2019 22:05

Extracting common devtools from Quarkus core repo

addcaeb

Exclude sisu inject from quarkus-core-deployment

c92f285

gsmet force-pushed the platform-resolver branch from e180646 to c92f285 Compare October 21, 2019 20:05

gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 21, 2019

gsmet merged commit a6f335f into quarkusio:master Oct 21, 2019

gsmet removed the backport? label Oct 21, 2019

gsmet modified the milestones: 0.27.0, 0.26.0 Oct 21, 2019

Extracting common devtools from Quarkus core repo #4615

Extracting common devtools from Quarkus core repo #4615

Uh oh!

Conversation

aloubyansky commented Oct 16, 2019

Uh oh!

ia3andy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoand commented Oct 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxandersen commented Oct 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsmet commented Oct 21, 2019

Uh oh!

gsmet commented Oct 21, 2019

Uh oh!

gsmet commented Oct 21, 2019

Uh oh!

aloubyansky commented Oct 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants