Skip to content

Add and use Metadata in ConsulDiscoveryOptions#1167

Merged
TimHess merged 7 commits into
SteeltoeOSS:mainfrom
cieciurm:feature/consul-add-metadata
Sep 8, 2023
Merged

Add and use Metadata in ConsulDiscoveryOptions#1167
TimHess merged 7 commits into
SteeltoeOSS:mainfrom
cieciurm:feature/consul-add-metadata

Conversation

@cieciurm
Copy link
Copy Markdown
Contributor

@cieciurm cieciurm commented Aug 27, 2023

Description

Hello!

Thank you for delivering Steeltoe 😄 .

I was browsing through the good first issue label and wanted to contribue a change for #438.

I'd like to open discussions on the goals and some details regarding implementation before executing it without the alignment.

Here are my thoughts:

  1. Currently ConsulDiscoveryOptions has a property called Tags (list of strings)
  2. Tags are expected to be in format key=value
  3. Internally some tags are added (such as group, instance zone)
  4. Currently the fallback from Tags to Metadata happens in ConsulRegistration:
  Metadata = ConsulServerUtils.GetMetadata(agentServiceRegistration.Tags);

Here are my questions:

  1. Would you like to keep the current behavior of usings tags as metadata? Should it be controlled by an option in ConsulDiscoveryOptions, f.e. for backwards compatibility?
  2. I guess, you like to keep adding the internal metadata (such as group, instance zone)
  3. What's the naming preference in ConsulDiscoveryOptions - should it be Metadata (well-known term) or Meta (Consul's AgentServiceRegistration propety is called Meta)

I hope, it's not too much - but I wanted to clarify before jumping to code immediately.

Once confirmed and agreed, I will finish the implementation (obviously including tests etc.).

Thanks in advance 😄 !

Fixes #438

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

@hananiel
Copy link
Copy Markdown
Contributor

hananiel commented Aug 29, 2023

Thank you for this PR. Since this PR is targeting main which will be release 4.x , we can deprecate the previous functionality with parsing tags as metadata. For functionality such as default groups, as well as property names, we should match as closely as possible the functionality described here: so Metadata for us. The prefix stays same as what is currently i.e. consul:discovery
If we back port to 3.x, we can consider a flag such as 'tagsAsMetadata'

@cieciurm
Copy link
Copy Markdown
Contributor Author

Thanks for your response, @hananiel!

I've completed my first attempt at the implementation. Scope:

  • Added the Metadata to ConsulDiscoveryOptions
  • Switched the metadata generation (including the internal generated ones) from the original metadata instead of Tags
  • Also exposed regular Tags to ConsulServiceInstance
  • I've adjusted unit tests

Please let me know if something needs to be adjusted or changed.

Thanks a lot in advance!

@cieciurm cieciurm changed the title [WIP] Add and use Metadata in ConsulDiscoveryOptions Add and use Metadata in ConsulDiscoveryOptions Aug 31, 2023
Copy link
Copy Markdown
Contributor

@hananiel hananiel left a comment

Choose a reason for hiding this comment

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

@cieciurm This looks good. Would you like to take a crack at updating the docs as well? The v4 docs are on this branch: https://github.com/SteeltoeOSS/Documentation/tree/v4

@cieciurm
Copy link
Copy Markdown
Contributor Author

cieciurm commented Sep 8, 2023

Thanks, @hananiel! You got it!

SteeltoeOSS/Documentation#302

@TimHess TimHess added Type/enhancement New feature or request Component/Discovery Issues related to Steeltoe Service Discovery ReleaseLine/4.x Identified as a feature/fix for the 4.x release line labels Sep 8, 2023
@TimHess TimHess added this to the 4.0.0-m1 milestone Sep 8, 2023
@TimHess TimHess merged commit 7322e3e into SteeltoeOSS:main Sep 8, 2023
@cieciurm cieciurm deleted the feature/consul-add-metadata branch September 8, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component/Discovery Issues related to Steeltoe Service Discovery ReleaseLine/4.x Identified as a feature/fix for the 4.x release line Type/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Consul client to use Metadata instead of Tags

3 participants