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

Refactor logic when creating extension via CLI #1300

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

TimonCzech
Copy link

@TimonCzech TimonCzech commented Sep 11, 2024

Summary

Refactored the code so it now creates a new extension request with the version 999-snapshot

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Release (follows conventions described in the RELEASE.md)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik
Copy link
Member

Thanks @TimonCzech , I am not sure, but don't have time to invest ATM. @jedla97 please check thoroughly

@@ -30,7 +30,7 @@ public class QuarkusCliClient {
public static final String COMMAND_LOG_FILE = "quarkus-cli-command.out";
public static final String DEV_MODE_LOG_FILE = "quarkus-cli-dev.out";

private static final String QUARKUS_VERSION_PROPERTY_NAME = "quarkus.version";
private static final String QUARKUS_VERSION_PROPERTY_NAME = "quarkus.platform.version";
Copy link
Member

Choose a reason for hiding this comment

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

let's think about it:

  • either we don't need to set quarkus.version, in which case let's drop it
  • or we need to set it
  • or both properties does same, but one report annoying message
  • or both fills purpose why we set them but one of them do more than the other, in which case, which one and what?

@mjurc this was just last month #1246, do you happen to remember details why is it necessary to have quarkus.version same in Quarkus CLI as in Quarkus app? I think it was about having 999-SNASHOT CLI creating app that we later build with 999-SNAPSHOT. When I create app quarkus-snapshot create app and look into pom, there is indeed quarkus.platform.version. Do you happen to remember why it was quarkus.version? I don't :-)

Copy link
Member

Choose a reason for hiding this comment

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

This is actually for CLI generated extensions. Extensions are part of platform, so they do not depend on platform, because that would create funny chicken and egg problems where extension depends on platform and platform depends on extensions. Instead, they depend on core bom.

So, extensions generated by Quarkus CLI have something like this in their pom:

    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>io.quarkus</groupId>
                <artifactId>quarkus-bom</artifactId>
                <version>${quarkus.version}</version>
                <type>pom</type>
                <scope>import</scope>
            </dependency>
        </dependencies>
    </dependencyManagement>

And this is why that property has to be -Dquarkus.version, otherwise the version in snapshot builds will be latest released core version available in generally available registry.

Anyway, this is kinda easy to reproduce also (if you don't wanna dig up what the test does:

git clone [email protected]:quarkusio/quarkus.git && pushd quarkus
./mvnw clean install -Dquickly
popd
cat <<EOF > ./quarkus-dev-cli
#!/bin/bash
java -jar ${PWD}/quarkus/devtools/cli/target/quarkus-cli-999-SNAPSHOT-runner.jar "\$@"
EOF
chmod +x ./quarkus-dev-cli
./quarkus-dev-cli version
./quarkus-dev-cli create extension extension-abc

Then check the extension-abc/pom.xml

Copy link
Member

Choose a reason for hiding this comment

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

Oh so it was for extension. Thanks for that context, I miss that.

Just thinking aloud, but wouldn't be better to create extension with --platform-bom=io.quarkus::999-SNAPSHOT as we doing it with apps? Tried it and it works + it should be supported by help.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would, if it works :)

Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Looking at the log I see that it's testing it with 999-SNAPSHOT.

It's adding --platform-bom=io.quarkus::999-SNAPSHOT to command here and extension integration test start 999-SNAPSHOT Quarkus here. So it's work the same way as it work before, just now we creating the extension with quarkus.version equal to 999-SNAPSHOT and don't overwrite it when building extension.

@TimonCzech can you update the PR description and title and commit description with your new changes otherwise it's look good

@TimonCzech TimonCzech changed the title Fixed warning for unrecognizable version when building quarkus via CLI Refactor logic when creating extension via CLI Sep 17, 2024
@jedla97
Copy link
Member

jedla97 commented Sep 17, 2024

CI is green, so merging.

@jedla97 jedla97 merged commit d686d69 into quarkus-qe:main Sep 17, 2024
8 checks passed
@michalvavrik michalvavrik mentioned this pull request Sep 17, 2024
11 tasks
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.

4 participants