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

Initial gRPC CLI #41249

Merged
merged 3 commits into from
Aug 12, 2024
Merged

Initial gRPC CLI #41249

merged 3 commits into from
Aug 12, 2024

Conversation

alesj
Copy link
Contributor

@alesj alesj commented Jun 17, 2024

Simple gcurl-like CLI -- currently supporting gRPC list, describe and invoke.

@alesj alesj requested a review from cescoffier June 17, 2024 11:05
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/grpc gRPC labels Jun 17, 2024
@alesj alesj requested a review from iocanel June 17, 2024 11:06
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

PR looks good to me.

@iocanel
Copy link
Contributor

iocanel commented Jun 18, 2024

It's not clear to me why CI barfs.

@alesj
Copy link
Contributor Author

alesj commented Jun 19, 2024

@gsmet any idea? ^

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

We need to verify if it can be built from scratch.

Also, Some tests would be useful.

import picocli.CommandLine.Spec;

@TopCommand
@Command(name = "grpc", sortOptions = false, header = "grpc CLI", subcommands = {
Copy link
Member

Choose a reason for hiding this comment

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

Why CLI in the header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just dropping the CLI?

@Spec
protected CommandSpec spec;

@CommandLine.Option(names = { "-h", "--help" }, usageHelp = true, description = "Display this help message.")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this generated by picocli automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, no idea -- copied this from one of the existing CLI commands ...
@iocanel ^ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iocanel can this be dropped? ^

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it can be possibly dropped.

quarkus.log.level=INFO
quarkus.banner.enabled=false
quarkus.package.jar.type=uber-jar
quarkus.package.jar.add-runner-suffix=false
Copy link
Member

Choose a reason for hiding this comment

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

This won't work I believe.
When building Quarkus from scratch, you won't find the plugin, because the plugin is built.

In another case, I had to switch to the assembly maven plugin (would love to switch back to the Quarkus plugin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yeah, this could be a problem ... since my copied example was from Quarkiverse, hence made it more easy.
Any ideas / suggestions what can be done?

Copy link
Member

Choose a reason for hiding this comment

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

Yes :-) I'm going to push a PR today that does this.

Copy link
Member

Choose a reason for hiding this comment

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

is there a reason we do uberjar here? why not just reuse users most likely already downloaded deps for this cli?

Copy link
Member

Choose a reason for hiding this comment

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

@maxandersen seems like Uber-jar is the way to distribute self-contained commands. At least that's what I got from the doc and when I tried the "fast-jar" approach, it did not work (and anyway, fast jar would need quarkus before quarkus)

Copy link
Member

Choose a reason for hiding this comment

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

yeah okey - if the actual cli is a quarkus app and not "just" a java app you need it. It would be interesting if we had a way to make a jar that has only the classes that need changing and rest could just be fetched using dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

hmm - even uber-jar would need quarkus before quarkus would it not?

Copy link
Member

Choose a reason for hiding this comment

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

Using Quarkus for these commands works, but yes it might be overkill.

By using the assembly plugin it works because it waits for all dependencies to be built before building that fat jar. Having a plugin dependency is slightly more complicated and fail in all my attempts from scratch. Funny, I had the same issue more than 10 years ago where I had to have profiles to build the plugin and then the modules using the plugin.

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to build the Maven plugin right after core-deployment to make it available for other extensions.

@alesj
Copy link
Contributor Author

alesj commented Jun 24, 2024

We need to verify if it can be built from scratch.

Scratch? Wdym?

Also, Some tests would be useful.

Yeah ... is there some existing example of it?
So I don't re-discover water :-)

@cescoffier
Copy link
Member

Look at #41418

@alesj
Copy link
Contributor Author

alesj commented Jul 17, 2024

@cescoffier OK, added assembly + test(s)

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@alesj alesj force-pushed the grpc_cli1 branch 2 times, most recently from e95969f to 21acd4f Compare July 18, 2024 15:18
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

alesj and others added 3 commits August 11, 2024 15:24
I don't know how we can end up with duplicates in the JSON descriptor
but let's make sure it doesn't affect our build.
It is not an extension so it should not depend on extensions.
@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 11, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 11, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 5426aaa.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 11, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5426aaa.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

  • Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

@gsmet gsmet dismissed cescoffier’s stale review August 12, 2024 09:42

I think the comments were addressed. I will merge it to 3.14 and we can improve on it later.

@gsmet
Copy link
Member

gsmet commented Aug 12, 2024

I think we will need to take a big step back as to what is an extension or not in the gRPC stuff as we had numerous issues with that and, while we were able to work around them, some are still around.

I will merge it for 3.14 as I think it's an interesting addition and we can improve on it.

@gsmet gsmet merged commit c852c8c into quarkusio:main Aug 12, 2024
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Aug 12, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 12, 2024
Copy link

🙈 The PR is closed and the preview is expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants