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.

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

[BUGFIX] Added Service Meta support in configuration files #4047

Merged

Conversation

pierresouchay
Copy link
Contributor

Add support for Meta in Consul's configuration, since #3881 only added support for Meta using HTTP registration of services.

Fixes #4045

@pearkes pearkes added this to the 1.1.0 milestone Apr 20, 2018
Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Can you also add documentation for this? Additionally, there may be appropriate place on the services page for a sub-section (eg "Service and Tag Names with DNS") talking about use cases or examples for service metadata.

@banks
Copy link
Member

banks commented Apr 20, 2018

We also missed updating a couple of spots in the api client package: agent specific register (/agent/service/register) and service list (/agent/services) use different structs for serialisation in api/agent.go which need meta data and tests added too.

@pierresouchay do you want to add that in this PR or separately?

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

This looks great to me - there is one nitpicky change that we should fix up but I think it's good after that.

@@ -319,6 +319,7 @@ type ServiceDefinition struct {
Name *string `json:"name,omitempty" hcl:"name" mapstructure:"name"`
Tags []string `json:"tags,omitempty" hcl:"tags" mapstructure:"tags"`
Address *string `json:"address,omitempty" hcl:"address" mapstructure:"address"`
Meta map[string]string `json:"node_meta,omitempty" hcl:"meta" mapstructure:"meta"`
Copy link
Member

Choose a reason for hiding this comment

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

JSON tag node_meta seems like copy paste error here? should just be meta like the others.

Also interesting that this wasn't caught by a test case - on the surface it seems like your tests would exercise that but in fact even when config looks like JSON we parse it into an interface{} and then use mapstructure. So the JSON tags here may never actually be used in Consul, and I don't think we explicitly test native JSON parsing, but let's fix this so it doesn't bite someone one day!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@banks Yes, Added missing Tests case in next patch in agent/config/runtime_test.go for HCL/JSON parsing

@banks banks merged commit c8db140 into hashicorp:master Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants