Repository navigation
gcp - bigtable - list clusters and tables by instance name - #11228
Open
Thanatos9404 wants to merge 2 commits into
Open
Thanatos9404 wants to merge 2 commits into
Thanatos9404 wants to merge 2 commits into
Conversation
bigtable-instance-cluster and bigtable-instance-table built the parent path from the instance displayName, so listing failed with a 404 for any instance whose display name differs from its id. Map the parent from the instance name instead. It is already the full projects/<project>/instances/<id> path, so the overrides that rebuilt it from the default project and the display name are no longer needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 4/5
test_bigtable.pywon’t catch a project-mismatch regression because the replay session and instance use the same project. Use a different project for the parent instance and session.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tools/c7n_gcp/tests/test_bigtable.py">
<violation number="1" location="tools/c7n_gcp/tests/test_bigtable.py:27">
P2: These new tests never exercise a project mismatch: both the replay session and returned instance use `cloud-custodian`. Use a fixture whose parent instance belongs to a different project and a separate session project so regressions to `get_default_project()` fail.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| def test_bigtable_instance_cluster_query(self): | ||
| # The instance's displayName differs from its id, so clusters | ||
| # have to be listed under the instance name. | ||
| factory = self.replay_flight_data('bigtable-instance-cluster-query') |
Contributor
There was a problem hiding this comment.
P2: These new tests never exercise a project mismatch: both the replay session and returned instance use cloud-custodian. Use a fixture whose parent instance belongs to a different project and a separate session project so regressions to get_default_project() fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tools/c7n_gcp/tests/test_bigtable.py, line 27:
<comment>These new tests never exercise a project mismatch: both the replay session and returned instance use `cloud-custodian`. Use a fixture whose parent instance belongs to a different project and a separate session project so regressions to `get_default_project()` fail.</comment>
<file context>
@@ -21,6 +21,32 @@ def test_bigtable_instance_cluster_backup_query(self):
+ def test_bigtable_instance_cluster_query(self):
+ # The instance's displayName differs from its id, so clusters
+ # have to be listed under the instance name.
+ factory = self.replay_flight_data('bigtable-instance-cluster-query')
+ p = self.load_policy({
+ 'name': 'bigtable-instance-cluster',
</file context>
Contributor
Author
There was a problem hiding this comment.
The parent bigtable-instance resource is listed in the session's project (scope_template='projects/{}'), so an instance's name always carries that same project and a mismatch can't happen in practice. The new tests cover the case behind the 404 in #10664, where the instance's displayName differs from its id.
…stance-name # Conflicts: # tools/c7n_gcp/c7n_gcp/resources/bigtable.py
This branch has not been deployed
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
Fixes #10664
gcp.bigtable-instance-clusterandgcp.bigtable-instance-tablebuilt their parent path from the instance'sdisplayName, so whenever the display name differs from the instance id, the list call returns a 404 and the policy fails.Instances already carry the full path in
name. This sets the cluster resource'schild_enum_paramsto('name', 'parent'), matching the table resource, and removes both_get_child_enum_argsoverrides so the baseChildResourceManagermaps the parent. This also fixesbigtable-instance-cluster-backup, which lists clusters through the cluster resource.Tests
Added
test_bigtable_instance_cluster_query(the policy from the issue) andtest_bigtable_instance_table_query, with an instance whose id and display name differ. Both fail on main because the request goes to the display name path, and pass with this change. I don't have a GCP project, so their flight data is hand-written in the shape of the existing bigtable recordings.#10676 also touches
bigtable.py, so whichever lands second needs a small rebase.