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.

Sign up for GitHub

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

Jump to bottom

Add first version of Spring Cloud Config Client extension #7130

Merged
merged 1 commit into from
Feb 14, 2020
Merged

Add first version of Spring Cloud Config Client extension #7130

merged 1 commit into from
Feb 14, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Feb 11, 2020 •
edited
Loading

Fixes: #5662

@boring-cyborg boring-cyborg bot added the area/dependencies Pull requests that update a dependency file label Feb 11, 2020
@geoand geoand added the area/spring Issues relating to the Spring integration label Feb 11, 2020
@geoand geoand changed the title Start adding Spring Config Server Client extension Add first version of Spring Config Server Client extension Feb 12, 2020
@geoand geoand marked this pull request as ready for review February 12, 2020 10:48
@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2020

Actually I think this is pretty much ready to go now... I am sure there are some cases where tweaking will be needed (for example we might need to look at the profile usage), but I would consider this feature complete at this point and the quicker we get feedback on its usage in real scenarios, the quicker we can improve it.

@cmoulliard
Copy link

Is there a doc describing how to use it such as what spring proposes https://spring.io/guides/gs/centralized-configuration/ ?

@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2020

Not yet, that would be a next step after this is in. But I'll definitely check it out and make sure the application behaves in the same way as the Spring Boot application described there

gastaldi
gastaldi reviewed Feb 12, 2020
View reviewed changes
bom/runtime/pom.xml Outdated Show resolved Hide resolved
@gastaldi gastaldi changed the title Add first version of Spring Config Server Client extension Add first version of Spring Cloud Config Client extension Feb 12, 2020
gastaldi
gastaldi reviewed Feb 12, 2020
View reviewed changes
extensions/pom.xml Outdated Show resolved Hide resolved
@gastaldi
Copy link
Contributor

Don't forget to add your extension here: https://github.com/quarkusio/quarkus/blob/master/.github/boring-cyborg.yml#L246-L253

@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2020

Thanks for the suggestions @gastaldi!

@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2020

@gastaldi your comments should be addressed now.

FTR, I plan to make another pass at this tomorrow by reading the Spring Cloud Config docs and going through online examples to see what is missing. But I don't expect there is any real work left on this, just tweaks and polish I guess

gsmet
gsmet previously requested changes Feb 12, 2020
View reviewed changes
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 bunch of cosmetic comments.

core/deployment/src/main/java/io/quarkus/deployment/builditem/FeatureBuildItem.java Outdated Show resolved Hide resolved
extensions/spring-cloud-config-client/deployment/pom.xml Outdated Show resolved Hide resolved
extensions/spring-cloud-config-client/pom.xml Outdated Show resolved Hide resolved
extensions/spring-cloud-config-client/runtime/pom.xml Outdated Show resolved Hide resolved
extensions/spring-cloud-config-client/runtime/pom.xml Show resolved Hide resolved
...main/java/io/quarkus/spring/cloud/config/client/runtime/SpringCloudConfigClientRecorder.java Outdated Show resolved Hide resolved
.../java/io/quarkus/spring/cloud/config/client/runtime/SpringCloudConfigServerClientConfig.java Outdated Show resolved Hide resolved
...nsions/spring-cloud-config-client/runtime/src/main/resources/META-INF/quarkus-extension.yaml Show resolved Hide resolved
integration-tests/spring-cloud-config-client/pom.xml Outdated Show resolved Hide resolved
integration-tests/spring-cloud-config-client/pom.xml Outdated Show resolved Hide resolved
@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2020

The darn rename was pretty bad... I obviously didn't think it through when I agreed to perform it :P

@geoand geoand requested a review from gsmet February 12, 2020 20:48
@geoand
Copy link
Contributor Author

geoand commented Feb 13, 2020

By going through the Spring Cloud Server documentation I have identified a few points where the current implementation differs from the Spring Cloud, so I'll address those differences over the next few hours

@geoand
Copy link
Contributor Author

geoand commented Feb 13, 2020

I just pushed what I consider to be a feature complete first version. I would be grateful for a new round of reviews

@geoand geoand added this to the 1.3.0 milestone Feb 13, 2020
gastaldi
gastaldi approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM, it would be nice to add a guide showing how to use it (and add the link to the quarkus-extension.yaml) when possible

@geoand
Copy link
Contributor Author

geoand commented Feb 13, 2020

LGTM, it would be nice to add a guide showing how to use it (and add the link to the quarkus-extension.yaml) when possible

Yes absolutely, that's on my list to be done before 1.3 goes out

@geoand
Copy link
Contributor Author

geoand commented Feb 13, 2020 •
edited
Loading

@gsmet is this OK for you as well?

@gastaldi
Copy link
Contributor

Curious if we get this in time for 1.3.0.Alpha2?

@geoand
Copy link
Contributor Author

geoand commented Feb 14, 2020

That has my +1 😉

@gastaldi gastaldi dismissed gsmet’s stale review February 14, 2020 21:27

Changes addressed

@gastaldi gastaldi merged commit e245565 into quarkusio:master Feb 14, 2020
@gastaldi gastaldi deleted the config-server-client branch February 14, 2020 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/core area/dependencies Pull requests that update a dependency file area/spring Issues relating to the Spring integration release/noteworthy-feature
Projects
None yet
Milestone
1.3.0.Alpha2
Development

Successfully merging this pull request may close these issues.

Spring Config support
4 participants
@geoand @cmoulliard @gastaldi @gsmet