[Misc] Plugin: providerSubaccountId runtime support added#85
[Misc] Plugin: providerSubaccountId runtime support added#85
providerSubaccountId runtime support added#85Conversation
|
|
||
| type btp struct { | ||
| GlobalAccountId string `json:"globalAccountId"` | ||
| GlobalAccountId string `json:"globalAccountId,omitempty"` |
There was a problem hiding this comment.
I was thinking perhaps we should for now leave both as required instead, but maybe optional is better for now?
minor enhancement to source `providerSubaccountId`. - misc adjustments until the field becomes mandatory (soon).
Pavan-SAP
force-pushed
the
provider-appId
branch
from
2da0841 to
37d7907
Compare
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The review has identified a logic inconsistency between the plugin's validation logic and the schema definitions. The globalAccountId field is treated as required in the JavaScript plugin but made optional in the schema files, creating a mismatch that could cause confusion or unexpected validation failures.
PR Bot Information
Version: 1.17.61 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Event Trigger:
pull_request.opened - Correlation ID:
739f8540-0e76-11f1-832a-48ca17e8817a - LLM:
anthropic--claude-4.5-sonnet
| ? ['appName', 'capOperatorSubdomain', 'clusterDomain', 'globalAccountId', 'providerSubaccountId'] | ||
| : ['appName', 'capOperatorSubdomain', 'clusterDomain', 'globalAccountId', 'providerSubaccountId', 'providerSubdomain', 'tenantId'] |
There was a problem hiding this comment.
Logic Error: Inconsistency in globalAccountId required status
The globalAccountId is listed as a required field here (lines 187-188), but in the schema files (files/chart/values.schema.json and files/configurableTemplatesChart/values.schema.json), it has been removed from the required array, and in hack/schema-generation.go:47 it's marked with omitempty. This creates an inconsistency where the plugin will reject YAML files missing globalAccountId even though the schema allows it to be optional.
Either globalAccountId should remain required in all places, or it should be made optional everywhere. Based on the PR description mentioning "misc adjustments until the field becomes mandatory (soon)", it appears both fields should be optional for now.
| ? ['appName', 'capOperatorSubdomain', 'clusterDomain', 'globalAccountId', 'providerSubaccountId'] | |
| : ['appName', 'capOperatorSubdomain', 'clusterDomain', 'globalAccountId', 'providerSubaccountId', 'providerSubdomain', 'tenantId'] | |
| const requiredFields = isServiceOnly | |
| ? ['appName', 'capOperatorSubdomain', 'clusterDomain', 'providerSubaccountId'] | |
| : ['appName', 'capOperatorSubdomain', 'clusterDomain', 'providerSubaccountId', 'providerSubdomain', 'tenantId'] |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Add
providerSubaccountIdRuntime Support to CAP Operator PluginNew Features
✨ Added support for the
providerSubaccountIdfield across the CAP Operator plugin to prepare for its transition to a mandatory field in upcoming releases.Changes
This pull request introduces the
providerSubaccountIdparameter throughout the CAP Operator plugin infrastructure, ensuring it is properly handled in runtime value generation, schema validation, and template files.bin/cap-op-plugin.js: AddedproviderSubaccountIdto required fields for both service-only and full application charts, and included a new interactive prompt to capture the provider sub-account ID during runtime value generation.files/chart/values.yaml.hbs&files/configurableTemplatesChart/values.yaml.hbs: AddedproviderSubaccountIdfield with documentation comment to the BTP configuration section, changingglobalAccountIdfrom mandatory (with inline comment) to optional.files/chart/values.schema.json&files/configurableTemplatesChart/values.schema.json: RemovedglobalAccountIdfrom the required fields array in the BTP schema definition and addedproviderSubaccountIdas an optional string property.files/runtime-values.yaml.hbs: AddedproviderSubaccountIdinterpolation to the BTP section of the runtime values template.hack/schema-generation.go: Modified thebtpstruct to makeGlobalAccountIdoptional (usingomitemptytag) while keepingProviderSubaccountIdas an optional field.test/cap-op-plugin.test.js: Updated all test cases to include the newproviderSubaccountIdprompt response, adjusting the call sequence indices for subsequent prompts across multiple chart type scenarios (standard, configurable templates, service-only, and IAS variants).Test fixture files (
test/files/expectedChart/*.yaml,test/files/expectedConfigurableTemplatesChart/*.yaml,test/files/*.yaml): Updated all expected output files and test input files to include theproviderSubaccountIdfield in the BTP configuration section, ensuring consistency across all test scenarios.📬 Subscribe to the Hyperspace PR Bot DL to get the latest announcements and pilot features!
PR Bot Information
Version:
1.17.61| 📖 Documentation | 🚨 Create Incident | 💬 Feedback22c55c60-0e78-11f1-93f0-b05e7cda2838issue_comment.edited