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

Stork Quickstart #1028

Merged

Conversation

cescoffier
Copy link
Member

Related to quarkusio/quarkus#22592

Checklist:

Your pull request:

  • targets the development branch
  • uses the 999-SNAPSHOT version of Quarkus
  • has tests (mvn clean test)
  • works in native (mvn clean package -Pnative)
  • has native tests (mvn clean verify -Pnative)
  • makes sure the associated guide must not be updated
  • links the guide update pull request (if needed)
  • updates or creates the README.md file (with build and run instructions)
  • for new quickstart, is located in the directory component-quickstart
  • for new quickstart, is added to the root pom.xml and README.md

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Added a few comments and questions.

<dependency>
<groupId>io.smallrye.stork</groupId>
<artifactId>smallrye-stork-service-discovery-consul</artifactId>
<version>1.0.0.Beta1</version>
Copy link
Member

Choose a reason for hiding this comment

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

Again, maybe let's add these versions to the BOM?

Copy link
Member Author

Choose a reason for hiding this comment

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

Waiting for Stork 1.0 to do so. It should not be long...

@@ -0,0 +1,140 @@
<?xml version="1.0"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Could you reformat with 4 spaces? That's what we're trying to do with your POMs.

Comment on lines 3 to 4
quarkus.rest-client.extensions-api.shared=true
quarkus.rest-client.extensions-api.name=fojooh
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 normal?

Copy link
Member

Choose a reason for hiding this comment

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

sharing is an advanced feature IMO and I don't think this should be added to the quickstart

Copy link
Member Author

Choose a reason for hiding this comment

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

Left-overs from another branch.

@michalszynkiewicz
Copy link
Member

I think it would be good to add a command to start Consul somewhere. Maybe some script or docker-compose

@cescoffier cescoffier force-pushed the add-stork-quickstart branch from d76665e to 3371bd8 Compare January 9, 2022 18:05
@cescoffier
Copy link
Member Author

cescoffier commented Jan 9, 2022

All done (reformated the pom and remove the rest client shared attribute).
I also added the consul docker command.

@cescoffier cescoffier force-pushed the add-stork-quickstart branch from 3371bd8 to af9e396 Compare January 9, 2022 18:07
@gsmet
Copy link
Member

gsmet commented Jan 9, 2022

@michalszynkiewicz I let you do a final check and merge it if all is good.

@michalszynkiewicz michalszynkiewicz merged commit 1c41064 into quarkusio:development Jan 10, 2022
@cescoffier cescoffier deleted the add-stork-quickstart branch January 10, 2022 06:37
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.

3 participants