Add support for tags #59
Conversation
justenwalker
force-pushed
the
service_tags
branch
from
b4df052
to
7b863d7
Compare
| @@ -65,6 +65,7 @@ type ServiceConfig struct { | |||
| Port int `json:"port"` | |||
| TTL int `json:"ttl"` | |||
| Interfaces json.RawMessage `json:"interfaces"` | |||
| Tags []string `json:"tags,omitempty"` | |||
There was a problem hiding this comment.
I think we're going to need to include tags for the BackendConfig as well. It's my understanding that the primary use case for this that @misterbisson was talking about in #10 was being able to split out services for e.g. dev vs prod. So when we query Consul we're probably going to want to have a list of tags to send to checkHealth and then on to Health.Service so that we're optionally filtering by tag.
|
@tgross What would the filter behavior be for tags on a Backend. Would they filter with AND (Must have every tag) or with OR (must have one of the tags) Or to make things easier - should it just be 1 tag. Consul API defines server-side filtering on a tag: https://www.consul.io/docs/agent/http/health.html#health_service But it doesn't go into detail about if it parses a comma-separated list of tags, or if it expects just 1 tag. |
|
I checked the consul codebase for clarification. It just accepts 1 tag (ignoring case) and filters if that tag is not found. So I think we should just use a single tag. We could support multiple tags by requesting all services and post-filtering them, but we still have to figure out what behavior we want for that (AND or OR) |
|
I'm feeling like there's going to come a point where we want query-with-multiple-tags support but without a backend that supports it I feel that'd be YAGNI. It's easy enough to add it in later. |
| @@ -85,6 +87,15 @@ func TestValidConfigParse(t *testing.T) { | |||
| t.Errorf("Expected 3 args but got unexpected results: %v", args) | |||
| } | |||
|
|
|||
| expectedTags := []string{"tag1", "tag2"} | |||
There was a problem hiding this comment.
Can we make sure we have a test for empty tags too? Just to verify that we don't regress with it down the road?
|
Ok, this LGTM. |
For #10
Add support for
tagsin the service configuration. Didn't seem like too much work to add, so I'm afraid I may have glossed over some important details.Another set of eyes appreciated.