Skip to content

Provide an alternate naming convention for CB ids #687

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

Merged

Conversation

ryanjbaxter
Copy link
Contributor

Configuration properties cannot contain characters like hash, parens, or commas.

See spring-cloud/spring-cloud-circuitbreaker#141

@ryanjbaxter ryanjbaxter added the enhancement New feature or request label Mar 9, 2022
@ryanjbaxter ryanjbaxter added this to the 3.1.2 milestone Mar 9, 2022
Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

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

LGTM, only thing is the name. Since circuit breaker is already part of the property, maybe feign.circuitbreaker.alphanumericNameResolver.enabled or feign.circuitbreaker.alphanumericNames.enabled

@ryanjbaxter ryanjbaxter force-pushed the provide-alternate-cbname branch 2 times, most recently from 271af4f to 4ad6bb0 Compare March 9, 2022 17:25
@@ -166,10 +166,20 @@ public Targeter defaultFeignTargeter() {

@Bean
@ConditionalOnMissingBean(CircuitBreakerNameResolver.class)
@ConditionalOnProperty(value = "feign.circuitbreaker.alphanumericNames.enabled",
Copy link
Member

@spencergibb spencergibb Mar 9, 2022

Choose a reason for hiding this comment

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

We need to use - rather than camel case in auto configuration, so alphanumeric-names. Users can use relaxed binding

…uration via configuration properties

Configuration properties cannot contain characters like hash, parens, or commas
@ryanjbaxter ryanjbaxter force-pushed the provide-alternate-cbname branch from 4ad6bb0 to b90559a Compare March 9, 2022 17:37
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants