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

connect: expose an API endpoint to compile the discovery chain #6248

Merged
merged 8 commits into from Aug 2, 2019

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Jul 30, 2019

Fixes #6213
Fixes #6255

  • change how DiscoveryTarget serializes to be SNI-based instead (comment)
  • pull all target information into a toplevel map keyed by the ID value and reference them from nodes by that ID (like how node transitions work). This removes the need for DiscoveryTarget to be TextMarshalled
  • /v1/discovery-chain/<service>: GET lets you pass the evaluationDC as an URL parameter but nothing else more complex. Anything else goes over POST (like overrides)
  • don't expose evaluationNamespace over the API for now
  • add endpoint tests
  • docs (moved to add docs for /v1/discovery-chain endpoint #6273)
  • api parallel copies of structures
  • request chain via cache

@rboyer rboyer added the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Jul 30, 2019
@rboyer rboyer added this to the 1.6.0-rc1 milestone Jul 30, 2019
@rboyer rboyer requested a review from a team July 30, 2019 20:03
@rboyer rboyer self-assigned this Jul 30, 2019
banks
banks previously requested changes Aug 1, 2019
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.

Looks great. I have a couple of thoughts for minor changes.

It also needs docs - I'd like to see what the output looks like in practice as it's a little hard to imagine just from the structs being used but I guess we'd see that in docs.

Also API package equivalents are also important not to forget before we land and close the issue.

agent/discovery_chain_endpoint.go Outdated Show resolved Hide resolved
agent/discovery_chain_endpoint.go Outdated Show resolved Hide resolved
agent/discovery_chain_endpoint.go Outdated Show resolved Hide resolved
* removed some unnecessary compiler output
* remove configurability of OverprovisioningFactor
@rboyer rboyer force-pushed the compiled-discovery-chain-api branch from 72a55eb to 534a9f5 Compare August 2, 2019 16:04
@rboyer rboyer changed the base branch from simplifed-compiled-discovery-chain to release/1-6 August 2, 2019 16:04
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a few really minor requests.

agent/discovery_chain_endpoint.go Outdated Show resolved Hide resolved
agent/discovery_chain_endpoint.go Show resolved Hide resolved
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@rboyer rboyer dismissed banks’s stale review August 2, 2019 20:34

Matt approved instead as Paul is OOO.

@rboyer rboyer merged commit c395aff into release/1-6 Aug 2, 2019
@rboyer rboyer deleted the compiled-discovery-chain-api branch August 2, 2019 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants