Introducting new aws_rds_db_global_cluster table#2707
Introducting new aws_rds_db_global_cluster table#2707ppapishe wants to merge 4 commits intoturbot:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Steampipe table for querying AWS RDS Global Clusters, along with documentation and plugin registration.
Changes:
- Added
aws_rds_db_global_clustertable implementation (list/get, tags, endpoint). - Registered the new table in the AWS plugin.
- Added table documentation with example queries (Postgres + SQLite).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/tables/aws_rds_db_global_cluster.md | Adds user-facing docs and example queries for the new table. |
| aws/table_aws_rds_db_global_cluster.go | Implements the new table, including list/get and hydrators for tags/endpoint. |
| aws/plugin.go | Registers the new table in the plugin table map. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func tableAwsRDSGlobalCluster(_ context.Context) *plugin.Table { | ||
| return &plugin.Table{ | ||
| Name: "aws_rds_db_global_cluster", | ||
| Description: "AWS RDS Global Cluster", |
There was a problem hiding this comment.
The table factory function name is inconsistent with the existing RDS DB table naming pattern (e.g., tableAwsRDSDBCluster/tableAwsRDSDBOptionGroup). To match the convention and improve discoverability, rename this to tableAwsRDSDBGlobalCluster and update the reference in aws/plugin.go.
| Name: "global_cluster_arn", | ||
| Description: "The Amazon Resource Name (ARN) for the global database cluster.", | ||
| Type: proto.ColumnType_STRING, | ||
| }, |
There was a problem hiding this comment.
Most tables in this plugin expose the primary ARN as a column named arn (including other RDS tables). Exposing it here as global_cluster_arn makes cross-table querying less consistent. Consider renaming this column to arn (keeping akas as-is) and updating any docs/examples accordingly before release.
|
@ppapishe could you please take a look at the copilot review comments? |
misraved
left a comment
There was a problem hiding this comment.
Code Review
BLOCKER: Duplicate rows across regions
The table uses SupportedRegionMatrix(AWS_RDS_SERVICE_ID) with awsRegionalColumns(), but DescribeGlobalClusters returns the same global clusters from every region. This will produce duplicate rows for every configured region.
The table should either:
- Remove the region matrix and use
awsAccountColumns()(query from a single default region), or - Use
awsGlobalRegionColumns()with a single-region matrix (seeaws_budgets_budgetoraws_organizations_accountfor patterns)
This must be fixed before merge — without it the table is fundamentally broken for multi-region configurations.
SUGGESTION: Redundant transform on endpoint column
Transform: transform.FromField("Endpoint"),The plugin's default FromCamel() transform already handles this. Other columns in the same table (e.g., status, engine) correctly rely on the auto-transform. Remove for consistency.
SUGGESTION: Add optional KeyColumns for List
The DescribeGlobalClusters API supports a GlobalClusterIdentifier filter. Adding it as an optional KeyColumn would allow filter pushdown:
KeyColumns: []*plugin.KeyColumn{
{Name: "global_cluster_identifier", Require: plugin.Optional},
},SUGGESTION: PR body has placeholder text
The integration test section says "Add passing integration test logs here" and the example query section says "Add example SQL query results here". Actual test evidence should be included before merge.
NIT: Typo in PR title
"Introducting" → "Introducing"
NIT: Extra blank line before tags_src column
Minor formatting inconsistency in the column definitions.
Positives
- Correct pagination with
WaitForListRateLimit+RowsRemainingcheck - Proper tag hydrate pattern (
tags_srcraw +tagstransformed) - Good documentation with JSONB cross-join examples (both PostgreSQL and SQLite)
- Correct
plugin.goregistration in alphabetical order - Proper error handling with
shouldIgnoreErrors([]string{"GlobalClusterNotFoundFault"})
🤖 Generated with Claude Code
Integration test logs
Logs
Example query results
Results