-
Notifications
You must be signed in to change notification settings - Fork 3k
Check for invalid characters when adding an extension #50419
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
base: main
Are you sure you want to change the base?
Check for invalid characters when adding an extension #50419
Conversation
I'm not sure I'm fully happy with this as it still can lead to bad dependencies being added because when given a GAV, it just adds it, and you'll find out it's bad with your first Maven command. Regardless, some testing scenarios... java -jar quarkus-cli-999-SNAPSHOT-runner.jar ext add io.quarkiverse.businessscore:quarkus-business-score-health~:1.0.0.Alpha4
[ERROR] ❗ Nothing installed because keyword(s) 'io.quarkiverse.businessscore:quarkus-business-score-health~:1.0.0.Alpha4' were not matched in the catalog. Note an invalid dependency, but it has valid characters in it: java -jar quarkus-cli-999-SNAPSHOT-runner.jar ext add io.quarkus:quarkus-info-:3.20.1
[SUCCESS] ✅ Extension io.quarkus:quarkus-info-:3.20.1 has been installed Note a really invalid dependency: java -jar quarkus-cli-999-SNAPSHOT-runner.jar ext add group:artifact:version
[SUCCESS] ✅ Extension group:artifact:version has been installed If you leave off the version, then the dependency is validated against the catalog, so it would have given an error for adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked the code but is there a reason why we don't send an explicit error?
Thanks for having a look at this one btw :) |
Yeah, that would be nicer, right? Unfortunately, it goes back to I stopped short of adding that as it touches many more classes, but if you think it's worth it, I can take a look at that. |
f981842
to
7fcf461
Compare
@gsmet Meh, it was easier than I thought, so I just went ahead and added it. Example now: java -jar quarkus-cli-999-SNAPSHOT-runner.jar ext add io.quarkiverse.businessscore:quarkus-business-score-health~:1.0.0.Alpha4
[FAILURE] ❌ Nothing installed because keyword(s) 'io.quarkiverse.businessscore:quarkus-business-score-health~:1.0.0.Alpha4' were invalid. |
63aa9ad
to
cc50d1a
Compare
Can you include some tests to justify the change? |
...-common/src/main/java/io/quarkus/devtools/commands/handlers/AddExtensionsCommandHandler.java
Outdated
Show resolved
Hide resolved
Sure, just as soon as I figure out where to put 'em. |
This comment has been minimized.
This comment has been minimized.
* Fixes quarkusio#49895 * use a pattern matcher to validate the groupId and artifactId for a fully qualified extension name (group, artifact, and version) * introduced a new collection of invalid keywords so they can be reported separately from those that are not in the catalog * added some tests that try to mimic manual tests that have been used Signed-off-by:Nathan Erwin <[email protected]>
cc50d1a
to
39f9848
Compare
Ok @gastaldi I took a swipe at adding tests, maybe it's ok? Not really sure... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Status for workflow
|
Signed-off-by:Nathan Erwin [email protected]