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

cli: add services register and deregister #4732

Merged
merged 12 commits into from
Oct 2, 2018
Merged

cli: add services register and deregister #4732

merged 12 commits into from
Oct 2, 2018

Conversation

mitchellh
Copy link
Contributor

@mitchellh mitchellh commented Oct 1, 2018

This PR adds the commands consul services register and consul services deregister for registering and deregistering services respectively against the local agent API. These commands operate in a batch fashion, making it easy for scripts, hooks, etc.

Both commands accept both flags to identify services as well as standard Consul configurations. This makes it easy since a standard Consul service definition in HCL or JSON can be used directly with this command without changes.

Mapstructure Update

This PR also contains a vendor update to mapstructure to bump it to v1.1.1. This has important changes that make it easy to map the structs.ServiceDefinition to an api.AgentServiceRegistration mostly automatically.

@mitchellh mitchellh requested a review from a team October 1, 2018 16:46
@pearkes pearkes added this to the 1.3.0 milestone Oct 1, 2018
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 awesome!

I have one feature request that I think would be easy to add but can be done later: allow reading JSON/HCL service defintion from STDIN, perhaps using the convention that - as a file name indicates stdin.

This would be really useful when using this to generate complex registrations (e.g. connect proxies with upstreams) from scripts - you can just echo and pipe rather than having to create temp files or names pipes and clean them up again.

I really missed this in envoy in the current envoy command work - if envoy supported this for bootstrap it would make that wrapper tool way cleaner - no passing secrets through filesystem or CLI args etc. Note that our service definitions can also contain ACL tokens so providing a more secure way to pass them from a config generating script rather than writing to a temp file is just as relevant here.

command/services/config.go Show resolved Hide resolved
},
},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

When f-envoy lands it might be worth adding explicit test cases for connect proxys (including upstreams etc) and sidecar services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, that's also why I prepped the table test. :D

}

if err := client.Agent().ServiceDeregister(id); err != nil {
c.UI.Error(fmt.Sprintf("Error registering service %q: %s",
Copy link
Member

Choose a reason for hiding this comment

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

Why %q for a string? Not a big deal just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes for cleaner output to the user to see a quoted string, it also helps in cases where there are spaces (a service name shouldn't have spaces but just in general). So usually when erroring out to the user or even logging (w/o hclog) I use %q in places I'd otherwise use %s.

@mitchellh mitchellh merged commit 868cb6d into master Oct 2, 2018
@mitchellh mitchellh deleted the f-service-cli branch October 2, 2018 19:46
@mitchellh mitchellh mentioned this pull request Oct 4, 2018
1 task
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.

3 participants