🌱 Return *DeploymentConfig directly from GetDeploymentConfig()#2598
🌱 Return *DeploymentConfig directly from GetDeploymentConfig()#2598joelanford wants to merge 1 commit intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR simplifies how deployment configuration is accessed by changing Config.GetDeploymentConfig() to return a typed *DeploymentConfig (with an error) directly, removing the need for callers to convert from a generic map[string]any.
Changes:
- Updated
GetDeploymentConfig()to unmarshal intoDeploymentConfiginternally and return(*DeploymentConfig, error). - Removed
convertToDeploymentConfig()and updated the applier to consume the typed config directly. - Updated unit tests to assert against
*config.DeploymentConfigand the new error return.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/operator-controller/config/config.go | Changes GetDeploymentConfig() to return (*DeploymentConfig, error) and unmarshal internally. |
| internal/operator-controller/applier/provider.go | Removes map→struct conversion helper; uses the new accessor directly. |
| internal/operator-controller/config/config_test.go | Updates expectations for the new return type and error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
05fd6db to
35c51b7
Compare
|
/hold Still self-reviewing Claude's work and making sure I address Copilot's feedback. |
35c51b7 to
a6ed8a8
Compare
a6ed8a8 to
3779279
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Change GetDeploymentConfig() to return (*DeploymentConfig, error) instead of map[string]any, eliminating the intermediate convertToDeploymentConfig() function in provider.go. The caller was immediately converting the map to a DeploymentConfig anyway, so this simplifies the API and removes unnecessary indirection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3779279 to
bb588d5
Compare
|
/hold cancel |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2598 +/- ##
==========================================
+ Coverage 67.81% 67.96% +0.15%
==========================================
Files 137 137
Lines 9588 9575 -13
==========================================
+ Hits 6502 6508 +6
+ Misses 2586 2573 -13
+ Partials 500 494 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
GetDeploymentConfig()previously returnedmap[string]any, requiring callers toimmediately convert it to a
*DeploymentConfigvia a separateconvertToDeploymentConfig()function. This changes the method to return
(*DeploymentConfig, error)directly,eliminating the unnecessary indirection.
Changes:
config/config.go:GetDeploymentConfig()now returns(*DeploymentConfig, error)instead ofmap[string]any, unmarshaling directly into the struct internallyapplier/provider.go: RemovedconvertToDeploymentConfig()function; caller now uses the*DeploymentConfigdirectlyconfig/config_test.go: Updated test expectations for new return type; removed defensive-copy test (no longer applicable to struct return)Reviewer Checklist
Generated with Claude Code