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 multiple tags for health and catalog http api endpoints #4717

Merged
merged 4 commits into from
Oct 11, 2018

Conversation

adilyse
Copy link
Contributor

@adilyse adilyse commented Sep 26, 2018

Fixes #1781.

Adds a ServiceTags field to the ServiceSpecificRequest to support
multiple tags, updates the filter logic in the catalog store, and
propagates these change through to the health and catalog endpoints.

Note: Leaves ServiceTag in the struct, since it is being used as
part of the DNS lookup, which in turn uses the health check. This
singular field takes precedence over anything in ServiceTags.

@adilyse adilyse requested a review from a team September 26, 2018 17:19
mkeeler
mkeeler previously requested changes Oct 3, 2018
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.

This looks good to me. My one request would be to use the require module in the new tests. I commented on it in a couple places but then realized it was everywhere so I didn't bother adding comments to every location.


nodes := out2.Nodes
if len(nodes) != 1 {
t.Fatalf("Bad: %v", nodes)
Copy link
Member

Choose a reason for hiding this comment

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

For new tests I would recommend importing github.com/stretchr/testify/require and using the functions in there.

This particular block:

if len(nodes) != 1 {
   t.Fatalf("Bad: %v", nodes)
}

Can be replaced with:

require.Len(t, nodes, 1)

We already use this in many other places and its more ergonomic than tons of the if / Fatalf blocks

agent/consul/state/catalog_test.go Outdated Show resolved Hide resolved
@adilyse adilyse force-pushed the feature/multiple-tag-filters-1781 branch 2 times, most recently from 7ae8fd8 to 84012ce Compare October 9, 2018 22:57
Fixes #1781.

Adds a `ServiceTags` field to the ServiceSpecificRequest to support
multiple tags, updates the filter logic in the catalog store, and
propagates these change through to the health and catalog endpoints.

Note: Leaves `ServiceTag` in the struct, since it is being used as
part of the DNS lookup, which in turn uses the health check.
@adilyse adilyse force-pushed the feature/multiple-tag-filters-1781 branch from 84012ce to 98cdcaa Compare October 10, 2018 23:03
@adilyse adilyse added this to the 1.3.0 milestone Oct 10, 2018
@banks banks dismissed mkeeler’s stale review October 11, 2018 11:48

Was addressed but @mkeeler's not online yet

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.

All feedback adressed!

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