-
Notifications
You must be signed in to change notification settings - Fork 542
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
Reinstate tags-as-metadata adding option to merge tags. Fixes #699 #700
Conversation
f8d3e46
to
8e10f73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Enable merge consul tag data with metadata to create service instance (defaults | ||
* to false). | ||
*/ | ||
private boolean enableTagMerge = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to mergeTagsEnabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
import static org.springframework.cloud.consul.discovery.ConsulServerUtils.findHost; | ||
|
||
public class ConsulServiceInstance extends DefaultServiceInstance { | ||
|
||
private HealthService healthService; | ||
|
||
public ConsulServiceInstance(HealthService healthService, String serviceId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a public API so existing constructor signatures will need to remain with a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a constructor with default merge value as false
.
I just moved the default constructor to be the first in the file.
Looking into it. I will remove draft status when ready. @spencergibb on f672845 we should set both tags and metadata if the merge is enabled? Because before we had a property for tags or metadata. Now we have a property for tags and metadata. What I should do? |
Yes, I think it should be a merge. |
@lucasoares any chance to update this? |
Hello @spencergibb. Unfortunately, I had no time to check it. I will try to get back to it this weekend. |
Closing in favor of #775 |
Hello.
See #699 and linked issues for more information.
This is my first PR into spring-cloud-consul. To be honest I didn't explore the project due to lack of time so I wanted to make the few changes as possible. Please let me know if there is anything to change.
I also didn't had time to configure checkstyle so I will wait for for CI results.
Thanks.