-
Notifications
You must be signed in to change notification settings - Fork 209
chore: Add singular data source support to codespec generation #3970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Add DataSources struct to codespec model - Add DataSources config to config_model - Add datasources section to config.yml for alert_configuration_api - Update api_to_provider_spec_mapper for data source mapping - Update config transformations for data sources - Generate alert_configuration_api model with data source support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds data source generation support to the codespec system, enabling independent schema configuration for singular and plural data sources alongside existing resource definitions.
Key Changes:
- Adds
DataSourcesconfiguration block to support independent data source schema options (aliases, overrides) separate from resource schemas - Updates
APIOperationsto makeCreateandReadoperations optional via pointers, supporting data-source-only API resources - Implements data source attribute merging logic that properly handles path parameters and response attributes with alias support
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/codegen/models/org_service_account_secret_api.yaml | Removes unused update: null operation declaration |
| tools/codegen/models/alert_configuration_api.yaml | Adds comprehensive data source schema definition with read/list operations and attribute specifications |
| tools/codegen/config/config_model.go | Introduces DataSources struct to config model for independent data source generation |
| tools/codegen/config.yml | Adds data source configuration block with read/list operations and alias mapping for alert configurations |
| tools/codegen/codespec/model.go | Defines DataSources and DataSourceSchema structs; updates APIOperations to use pointers for optional operations |
| tools/codegen/gofilegen/resource/resource_file.go | Updates operation model conversion to handle pointer-based operations |
| tools/codegen/gofilegen/resource/resource_file_test.go | Updates test fixtures to use pointer-based APIOperation structs |
| tools/codegen/codespec/config.go | Implements data source transformation logic and refactors path parameter aliasing to support both resources and data sources |
| tools/codegen/codespec/config_test.go | Updates test fixtures to use pointer-based APIOperation structs |
| tools/codegen/codespec/api_to_provider_spec_mapper.go | Implements apiSpecToDataSourcesModel function and attribute merging logic for data source generation |
| tools/codegen/codespec/api_to_provider_spec_mapper_test.go | Updates test fixtures to use pointer-based APIOperation structs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AgustinBettati
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting this PR, helped a lot for review.
Overall LGTM, leaving some suggestions and questions for improvements.
|
|
||
| simpleTestResourceOperations = codespec.APIOperations{ | ||
| Create: codespec.APIOperation{ | ||
| Create: &codespec.APIOperation{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would add a unit test in which a config defines data sources configuration and we can assert the resulting model
|
|
||
| // Add path params as required (they identify the data source) | ||
| // Apply aliases to path params during merge | ||
| for i := range pathParams { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: For all merging logic, can we reuse existing addOrUpdate logic we use for resource model generation in merge_attributes.go?
We could also move data source merge function into this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think moving data source merge functions to merge_attributes.go makes sense, but I would keep separate merge functions, because data source logic is much simpler, and the addOrUpdate function is designed for resource-specific complexity (POST/PATCH/response merging) that doesn't apply to data sources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved in ca348a7
| // Path params are enforced as required; response attributes are marked computed. | ||
| // Aliases are applied to both path params and response attributes during merge to properly detect duplicates. | ||
| // If duplicates exist (same TFSchemaName after aliasing), Required always wins over Computed. | ||
| func mergeDataSourceAttributes(pathParams, responseAttrs Attributes, aliases map[string]string) Attributes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have a step during defined in dataSourceTransformations which calls aliasTransformation, I like this as it reuses all logic with resource model generation. Is there a reason why this function is also handling aliasing? Would strongly favour handling alias in a single place after all merging of attributes is done (similar to resource model generation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this differently to be able to handle the case where we want to alias a path param and a response attribute that are the same but have different names in the path and in the response. For example:
schema:
aliases:
alertConfigId: integrationId
id: integrationId
The problem with aliasing AFTER merge:
- Path param: alertConfigId → TFSchemaName: alert_config_id (Required)
- Response attr: id → TFSchemaName: id (Computed)
- After merge: both alert_config_id and id exist
- aliasTransformation changes alert_config_id → integration_id and id → integration_id
Result: TWO attributes with TFSchemaName = integration_id ❌
Why aliasing DURING merge works:
- Path param: alertConfigId → alias to integration_id (Required)
- Response attr: id → integration_id (Computed)
- During merge: duplicate detected → Required wins
- Result: ONE integration_id attribute (Required) ✅
The aliasTransformation doesn't handle duplicate detection, it just renames. So for data sources that we want to alias like in the example above, we need to handle it differently.
Let me know your thoughts, i wouldn't block this PR for this, so if we come up with a different solution, we can do it in a follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this more advanced aliasing feature only supported for data sources? My main doubts comes from diff between resource and data source generation, and hence the difference it what aliasing they support.
I would keep both consistent, we can create a CLOUDP to support this more advanced alias in both implementations and prioritise as we see the need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case only happens in the data source because we have the same field as a path param(required input) and as a response attribute(computed output), and there we want to align the name of both fields while avoiding duplicates. In the case of the resource, we only have it in the response attribute, so a computed field, and there's no possibility of duplicates. Hope it makes sense why we need it for the data source only
| return result | ||
| } | ||
|
|
||
| func sortAttributesRecursive(attrs *Attributes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we need this dedicated sorting step? For reference for resource generation we dont have a dedicated sorting step, buildResourceAttrs does a simple sorting when iterating over schema properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored this in ca348a7
| // Process List operation if defined | ||
| if dsConfig.List != nil { | ||
| // Extract list operation for description | ||
| if oasListOp, err := extractOp(spec.Paths, dsConfig.List); err == nil && oasListOp != nil { | ||
| pluralDescription = &oasListOp.Description | ||
| } | ||
|
|
||
| listOp = &APIOperation{ | ||
| HTTPMethod: dsConfig.List.Method, | ||
| Path: dsConfig.List.Path, // alias will be applied later by transformations helper | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now we only process path params of the read, but not of the list right? Would make clear in PR description what we support related to singular data source model and what is currently pending for the plural (knowing path params of list operation + query params). cc @maastha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to implement codespec generation for singular data source only. Changed the title to say singular data source explicitly
| attributes: | ||
| - string: {} | ||
| description: Uniform Resource Locator (URL) that points another API resource to which this response has some relationship. This URL often begins with `https://cloud.mongodb.com/api/atlas`. | ||
| computed_optional_required: optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spotted in other PR (#3971 (comment)), nested attributes are being marked as Optional/Required in the data source model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! working on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in f7f90de
| read: | ||
| http_method: GET | ||
| path: /api/atlas/v2/orgs/{orgId}/serviceAccounts/{clientId} | ||
| update: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also update the model for org service account so we know it works as expected?
| Create: codespec.APIOperation{}, | ||
| Read: codespec.APIOperation{}, | ||
| Create: &codespec.APIOperation{}, | ||
| Read: &codespec.APIOperation{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test here for aliasing as well for resource & data source?
Description
Adds support for codespec generation to the use it to support singular data source generation
Link to any related issue(s): CLOUDP-363703
Type of change:
Required Checklist:
Further comments