fix: resolve hostname/environment from plain Name tag, not just legacy prefix - #50
Merged
Merged
Conversation
…y prefix
Current cloudX-instance.yaml sets the Name tag to the plain hostname
with no cloudX-{env}- prefix (that prefix was dropped at some point).
get_instance_tags() only recognized the old prefixed format via regex,
so a plain Name tag (e.g. "erix") fell through to "does not match
format" and yielded no hostname at all, even though the
cloudX:environment tag on the same instance was perfectly usable for
environment resolution.
Now Name is used as the hostname whenever the legacy prefixed pattern
doesn't match, and the environment tag takes precedence over one
parsed out of a legacy Name tag when both are present - only warning
when they actively disagree, never in the routine case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The implementation/tests currently conflict with the PR’s stated “warning” behavior and the test plan omits a described no-Name environment-tag case.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates CloudXSetup.get_instance_tags() to correctly derive hostname/environment from EC2 tags when the Name tag is no longer in the legacy cloudX-{env}-{hostname} format, while preserving backward compatibility for older instances.
Changes:
- Fall back to using
Nameas the hostname when the legacycloudX-{env}-{hostname}pattern doesn’t match. - Prefer
cloudX:environment/cloudx:environment/Environmenttags over{env}parsed from legacyName, and surface conflicts. - Add a dedicated test module covering hostname/environment resolution scenarios.
File summaries
| File | Description |
|---|---|
cloudx_proxy/setup.py |
Adjusts hostname parsing and environment precedence/conflict reporting in get_instance_tags(). |
tests/test_instance_tags.py |
Adds new tests for the updated tag parsing and precedence rules. |
Review details
Suppressed comments (1)
tests/test_instance_tags.py:112
- The test plan mentions covering the case where an environment tag is present but the instance has no Name tag; this file currently doesn’t exercise that scenario. Also,
test_environment_tag_alone_is_sufficientisn’t actually "tag alone" because it includes a Name tag. Adding a no-Name case and renaming this test would align coverage and intent.
def test_environment_tag_alone_is_sufficient(self, setup, monkeypatch):
environment, hostname = get_tags(
setup, monkeypatch, {"Name": "erix", "cloudx:environment": "dta"}
)
assert (environment, hostname) == ("dta", "erix")
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+209
to
+214
| if env_from_name and env_from_tag and env_from_name != env_from_tag: | ||
| self.print_status( | ||
| f"Name tag implies environment '{env_from_name}' but the environment tag says " | ||
| f"'{env_from_tag}' - using '{env_from_tag}'", | ||
| False, 2 | ||
| ) |
Comment on lines
+95
to
+98
| out = capsys.readouterr().out | ||
| assert "FOO" in out and "BAR" in out | ||
| assert "✗" in out | ||
|
|
A Name-tag/environment-tag mismatch is resolved deterministically (the tag always wins) and execution continues normally, so it's not a failure - print_status(..., False, ...) renders the red ✗ failure symbol, which this codebase reserves for things that actually broke (see the "Using vault ... instead of default" warning a few lines below for the established pattern: warning()-colored text with the neutral ○ symbol via print_status(..., None, ...)). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cloudX-instance.yamlnow sets theNametag to the plain hostname with nocloudX-{env}-prefix (that prefix was dropped at some point).get_instance_tags()only recognized the old prefixed format, so a plainName(e.g.erix) produced no hostname at all — "does not match format" — even thoughcloudX:environmenton the same instance was perfectly usable.Nameis now used as the hostname directly whenever the legacycloudX-{env}-{hostname}pattern doesn't match. The legacy pattern is still tried first, so older/existing instances keep working exactly as before.cloudX:environment/cloudx:environment/Environmenttag over{env}parsed out of a legacyNametag, since the tag is the current, authoritative source. A mismatch between the two (e.g.Name=cloudX-FOO-erikwithcloudX:environment=BAR) is flagged as a warning, with the tag winning — this is the only case that gets flagged; the common cases (plainName, orName/tag in agreement) are silent.Test plan
tests/test_instance_tags.pycovers: plainName+ env tag (the motivating case), legacy prefixedNamewith and without a| {username}suffix, tag-vs-parsed-name conflict (warns, tag wins), agreement (no warning), env tag alone with noName, and neither present (None, None).uv run pytest— 262 passed, no regressions.uv run ruff check cloudx_proxy tests— clean.🤖 Generated with Claude Code