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 force-pushed the config-server-client branch from a1b3dd7 to a622434 Compare February 11, 2020 09:34
@geoand geoand force-pushed the config-server-client branch from a622434 to 6dc0705 Compare February 11, 2020 14:50
@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 force-pushed the config-server-client branch from 6dc0705 to 473de6d Compare February 12, 2020 10:42
@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
Contributor

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
@geoand geoand force-pushed the config-server-client branch from 473de6d to b2e4b07 Compare February 12, 2020 15:24
@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
@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 geoand force-pushed the config-server-client branch from b2e4b07 to 2dfc2c0 Compare February 12, 2020 20:22
@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.

@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 force-pushed the config-server-client branch from 2dfc2c0 to aaaa448 Compare February 12, 2020 20:46
@geoand geoand requested a review from gsmet February 12, 2020 20:48
@geoand geoand force-pushed the config-server-client branch from aaaa448 to bf02ff5 Compare February 13, 2020 08:12
@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 geoand force-pushed the config-server-client branch from bf02ff5 to 5641c82 Compare February 13, 2020 12:15
@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 force-pushed the config-server-client branch from 5641c82 to 23841cb Compare February 13, 2020 12:19
@geoand geoand added this to the 1.3.0 milestone Feb 13, 2020
@geoand geoand force-pushed the config-server-client branch from 23841cb to 84c8b2e Compare February 13, 2020 12:35
@geoand geoand force-pushed the config-server-client branch from 84c8b2e to 851bae4 Compare February 13, 2020 13:21
@geoand geoand force-pushed the config-server-client branch from 851bae4 to 15cdc7d Compare February 13, 2020 13:22
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