Skip to content
This repository was archived by the owner on Feb 18, 2025. It is now read-only.

Add DiscoveryIgnoreHostnameFilters and deprecate DiscoveryIgnoreReplicaHostnameFilters#346

Closed
sjmudd wants to merge 3 commits intoopenark:masterfrom
sjmudd:DiscoveryIgnoreHostnameFilters
Closed

Add DiscoveryIgnoreHostnameFilters and deprecate DiscoveryIgnoreReplicaHostnameFilters#346
sjmudd wants to merge 3 commits intoopenark:masterfrom
sjmudd:DiscoveryIgnoreHostnameFilters

Conversation

@sjmudd
Copy link
Collaborator

@sjmudd sjmudd commented Nov 14, 2017

It may be necessary to filter out accessing the master as it may not be reachable (e.g. if in an external network) so this change changes the use of DiscoveryIgnoreReplicaHostnameFilters to the more general name DiscoveryIgnoreHostnameFilters.

Use of the old variable name will still work but will generate a deprecation warning.

Additional extra checks are added to ensure that a host that we want to ignore really is ignored (the config may change on a running orchestrator)

  • contributed code is using same conventions as original code
  • code is formatted via gofmt (please avoid goimports)
  • code is built via ./build.sh
  • code is tested via go test ./go/...

…caHostnameFilters

It may be necessary to filter out accessing the master as it may not be
reachable (e.g. if in an external network) so this change changes the
use of DiscoveryIgnoreReplicaHostnameFilters to the more general name
DiscoveryIgnoreHostnameFilters.

Use of the old variable name will still work but will generate a
deprecation warning.

Additional extra checks are added to ensure that a host that we want to
ignore really is ignored (the config may change on a running orchestrator)
@sjmudd
Copy link
Collaborator Author

sjmudd commented Nov 15, 2017

Note: requires #298 or at least there are some issues seen if this patch is not applied first.

@chrisboulton
Copy link

@shlomi-noach We're super interested in this change for a couple of things in our environment.. Is there anything we can do to help you test this, and what sounds like the requirement of #298 (I've not connected the requirement of that one to this, yet) to get them brought down into an upcoming release?

@shlomi-noach shlomi-noach self-requested a review August 23, 2018 06:04
@shlomi-noach shlomi-noach self-assigned this Aug 23, 2018
@shlomi-noach
Copy link
Collaborator

This PR makes sense to me, but I'm having trouble with #298. @sjmudd looking deeper, I'm unsure how this PR depends on #298 ; I think it can be applied without #298

@dougfales
Copy link
Contributor

@sjmudd we are interested in using this setting, or a similar one, to also ignore hosts on specific ports (see #923). I was wondering, would you consider allowing this new setting to match against the full host:port instead of just the hostname? Also, is there something blocking this PR? It seems to improve the existing functionality of this setting quite a bit, so I'd love to see it get merged.

@sjmudd sjmudd closed this Dec 25, 2021
@sjmudd sjmudd deleted the DiscoveryIgnoreHostnameFilters branch December 25, 2021 20:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants