update Consul vendor code so that catalog.ServiceMultipleTags can be

[cstyan]

Fixes #4786

cc: @tariq1890 you've touched this code recently also @simonpasquier

Signed-off-by: Callum Styan [email protected]

[tariq1890]

Hi @cstyan thanks for letting me know about this PR. I don't remember working on consul SD and If any of my changes caused a bug, I apologize. Could you let me know where exactly I made changes?

In any case, always happy to help review ^__^

[cstyan]

🤦‍♂️ my bad @tariq1890, wrong person

cc: @iksaif

[iksaif]

Good to know that consul now supports this :). Do we really want to break compatibility with the old config here?

[simonpasquier]

I agree with @iksaif that although this SD mechanism isn't covered by the stability guarantees, it would be nicer to not break users immediately. Maybe support both tag: <string> and tags: [ <string>, ... ] (tags taking precedence), advertise tag as deprecated and remove it after a couple of releases.

[brian-brazil]

I'd prefer to avoid cruft building up in the code. This is a fairly new feature, and should only be in use by advanced users.

[cstyan]

@iksaif @simonpasquier good point

@brian-brazil it's unclear whether you're saying we shouldn't support searching for services with multiple tags, or just to not include tags and tag in the config

[brian-brazil]

I'm saying don't include both.

[cstyan]

Any opinions on how to proceed? I don't see a way to not break peoples old config for this change. We can't marshal a single string into an array of strings with go-yaml.

go-yaml/yaml#100

[brian-brazil]

I'd suggest breaking it, as new features come along in SDs I'm not sure we should be spending too much effort on avoiding breakage. Particularly for more advanced features.

[cstyan]

Fine by me. It's a pretty minor change required in users config files anyways.

[cstyan]

Do we want to move ahead with these changes as is?

[simonpasquier]

👍 as @brian-brazil is ok with the breaking change.

[cstyan]

okay, seems no one is opposed

[cstyan]

@brian-brazil bump

[cstyan]

@brian-brazil @iksaif updated, a service must now contain all tags we're watching to be added to watched services.

cc: @ashepelev

[brian-brazil]

👍

You've conflicts

[cstyan]

@brian-brazil thanks, fixed.

I really need to check if there's a way to have GH notify me when there's a merge conflict.

[simonpasquier]

LGTM. Re conflict notifications, I don't know of something like this but Prow (which we already use for prombench) has a needs-rebase plugin that puts a label on PRs with conflicts. This won't get you notified though.

[cstyan]

@brian-brazil PTAL :)

[brian-brazil]

Thanks!