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

Consider sharing structs from Consul's API package #9

Open
banks opened this issue Mar 3, 2020 · 1 comment
Open

Consider sharing structs from Consul's API package #9

banks opened this issue Mar 3, 2020 · 1 comment
Assignees

Comments

@banks
Copy link

banks commented Mar 3, 2020

In https://github.com/haproxytech/haproxy-consul-connect/blob/556c75e4dede0754103b0b39b4d3f523be90cc13/consul/config.go

I see some struct duplication from the ones in our api package. Is there a good reason to need that vs just embedding our api package structs (say for upstreams or TLS configs) in your own wrapper that can add any additional context you want to pass through?

It might be reasonable as it is, I'm just thinking that as we add features especially with discovery chain stuff and some of our L7 config models it seems like wasted maintenance effort to keep up with copying more and more fields over to duplicate structs rather than just embedding ours.

Concretely, the biggest concerns there are the Config maps in Service.Proxy and Upstream and also the other fields in Upstream like MeshGateway. We'll be adding lots more config flags for things like timeouts, retries, circuit breaking etc. in to these places and as it stands now you'll have to explicitly add code to plumb them every time in the /consul package rather than just passing through and letting the /haproxy package just map them to what it needs.

@pierresouchay
Copy link
Collaborator

@ShimmerGlass Any advice/reason on that?

@banks We will probably switch (aka very soon) to those structures unless @ShimmerGlass is not Ok when implementing #19

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