Discovery Client - Support "ClusterId" environment variable without prefix #247
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the Discovery Client to support both the prefixed (NIDISCOVERY_CLUSTERID) and prefix-less (CLUSTERID) environment variable names for specifying the Cluster ID, aligning with the Discovery Service's own behavior. This change improves robustness and compatibility when interacting with the Discovery Service.
Key changes:
- Introduced constants for the environment variable prefix and cluster ID name
- Added fallback logic to check both prefixed and non-prefixed environment variable names
- Includes inline comment noting future consideration for case-insensitive environment variable lookups
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...i.measurementlink.discovery.v1.client/src/ni/measurementlink/discovery/v1/client/_support.py
Show resolved
Hide resolved
...i.measurementlink.discovery.v1.client/src/ni/measurementlink/discovery/v1/client/_support.py
Outdated
Show resolved
Hide resolved
hunter-ni
requested review from
bkeryan,
dixonjoel and
jfriedri-ni
as code owners
| def _get_discovery_service_address() -> str: | ||
| cluster_id = os.environ.get("NIDISCOVERY_CLUSTERID") | ||
| # To support operating systems other than Windows (and match Discovery Service behavior), | ||
| # we would likely want to make this check case-insensitive. |
There was a problem hiding this comment.
How do we document this for users? If we use shout-case there, then all OSes have the same casing for this variable and we can use os.environ directly.
There was a problem hiding this comment.
I believe @nick-beer's original comment (precipitating this change + comment) was that we would want to match here whatever the Discovery Service itself would be anticipated to actually support, which would be the environment variable of this given name (with optional prefix) irrespective of case. I don't have any specific insight on the question of existing documentation.
There was a problem hiding this comment.
If the server is going to use PascalCase, then you're better off using PascalCase in the Python and LV packages so that it works regardless of whether environment variables are case-sensitive or case-insensitive. However, a lot of software avoids this situation by using all-caps for environment variables everywhere.
There was a problem hiding this comment.
To summarize (and confirm that I'm on the same page):
Nick's comment indicates that the Discovery Service itself is explicitly designed to look for this environment variable in a case-insensitive manner (regardless of operating system). To your point though, existing Discovery Service code does use PascalCase when it needs to reference ClusterId and the NIDiscovery prefix, so we can use that casing here for consistency with the server code.
What does this Pull Request accomplish?
This set of changes adds support for the
ClusterIdenvironment variable (without theNIDiscovery_prefix), given that the Discovery Service recognizes an environment variable of that prefix-less name as well.This change is being made in response to this comment on PR #242.
Why should this Pull Request be merged?
This PR adds greater robustness in properly handling an environment variable being set to specify the Cluster ID of the Discovery Service.
What testing has been done?
DiscoveryClientwas able to successfully interact with the Discovery Service that was launched with theClusterIdenvironment variable setDiscoveryClientwas still able to successfully interact with a Discovery Service that was launched with theNIDiscovery_ClusterIDenvironment variable