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

Support Consul EnableTagOverride #243

Closed
flyinprogrammer opened this issue Nov 17, 2016 · 5 comments
Closed

Support Consul EnableTagOverride #243

flyinprogrammer opened this issue Nov 17, 2016 · 5 comments

Comments

@flyinprogrammer
Copy link
Contributor

flyinprogrammer commented Nov 17, 2016

If I'm reading this correctly:
https://github.com/joyent/containerpilot/blob/master/glide.yaml#L24
It means we're using consul from this commit:
https://github.com/hashicorp/consul/tree/158eabdd6f2408067c1d7656fa10e49434f96480

Which means we're using consul bindings from 0.5.2
There have been a lot of updates since then (we're on 0.7.1 now), like EnableTagOverride
https://github.com/hashicorp/consul/blob/73b281a27c748210f64e4e83b23312d23afb4593/api/agent.go#L51

And I was hoping to add an option to use EnableTagOverride here:
https://github.com/joyent/containerpilot/blob/master/discovery/consul/consul.go#L127

Which would mean probably adding a field to the ServiceDefinition?
https://github.com/joyent/containerpilot/blob/master/discovery/discovery.go#L22
But I'm hesitant to do that because things like etcd have no concept of an EnableTagOverride flag. So I just don't know how we want to accomplish this.

I'm sorry I'm so new to this code base. I demand 😛 that I write the code and submit the patch, if this is a feature we want? (I need it.), so I would appreciate any and all feedback so I can start contributing more silly ideas.

@tgross
Copy link
Contributor

tgross commented Nov 17, 2016

There's no specific reason we've fallen behind on that, so updating the bindings sounds like a good idea. What would EnableTagOverride look like in the case where we have non-Consul backends?

Just for the record, because I had to look up what that does (from https://www.consul.io/docs/agent/services.html):

The enableTagOverride can optionally be specified to disable the anti-entropy feature for this service. If enableTagOverride is set to TRUE then external agents can update this service in the catalog and modify the tags. Subsequent local sync operations by this agent will ignore the updated tags.

@tgross tgross changed the title Consul still on 0.5.2 bindings? Update Consul bindings Nov 17, 2016
@tgross
Copy link
Contributor

tgross commented Nov 17, 2016

@flyinprogrammer I'd like to suggest that you make two PRs if you decide to take this on: one to upgrade Consul and a second one to tackle the question of EnableTagOverride

@tgross
Copy link
Contributor

tgross commented Nov 17, 2016

For EnableTagOverride I'm going to suggest you go with the proposal in #213 in terms of config syntax. i.e. namespace the configuration change.

@tgross tgross changed the title Update Consul bindings Support Consul EnableTagOverride Nov 18, 2016
@tgross
Copy link
Contributor

tgross commented Nov 22, 2016

Being implemented in #252

@tgross
Copy link
Contributor

tgross commented Dec 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants