HELM-611: add OCI registry client for chart operations #15830
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds GetDefaultOCIRegistry to initialize and attach an OCI registry client to a Helm action.Configuration, plus unit tests. Wires this function into the helmHandlers as a pluggable proxy and invokes it at the start of multiple HTTP handlers to ensure the registry client is available before REST client setup, returning 502 on failure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Comment |
openshift-ci
bot
added
component/backend
sowmya-sl
force-pushed
the
add-oci-support
branch
from
d7d0948 to
42f0ddd
Compare
|
/test-all |
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
sowmya-sl
force-pushed
the
add-oci-support
branch
from
42f0ddd to
4100050
Compare
|
/retest |
6 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/helm/actions/get_registry.go (1)
10-20: Consider checking for existing RegistryClient before overwriting.The function unconditionally assigns a new registry client to
conf.RegistryClientwithout checking if one already exists. While this is likely the intended behavior, a defensive check could prevent accidentally overwriting a previously configured client.If desired, apply this diff to add a defensive check:
func GetDefaultOCIRegistry(conf *action.Configuration) error { if conf == nil { return fmt.Errorf("action configuration cannot be nil") } + if conf.RegistryClient != nil { + return nil + } registryclient, err := registry.NewClient(registry.ClientOptDebug(false)) if err != nil { return fmt.Errorf("failed to create registry client: %w", err) } conf.RegistryClient = registryclient return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
pkg/helm/actions/get_registry.go(1 hunks)pkg/helm/actions/get_registry_test.go(1 hunks)pkg/helm/handlers/handler_test.go(2 hunks)pkg/helm/handlers/handlers.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/helm/actions/get_registry.gopkg/helm/handlers/handlers.gopkg/helm/handlers/handler_test.gopkg/helm/actions/get_registry_test.go
🧬 Code graph analysis (2)
pkg/helm/handlers/handlers.go (2)
pkg/helm/actions/get_registry.go (1)
GetDefaultOCIRegistry(10-20)pkg/serverutils/utils.go (2)
SendResponse(13-28)ApiError(87-89)
pkg/helm/actions/get_registry_test.go (2)
pkg/helm/handlers/handler_test.go (1)
FakeConfig(191-193)pkg/helm/actions/get_registry.go (1)
GetDefaultOCIRegistry(10-20)
🔇 Additional comments (12)
pkg/helm/handlers/handler_test.go (2)
63-68: LGTM!The wiring of
getDefaultOCIRegistryin the test helper follows the established pattern for other handler functions and correctly uses the fake implementation.
205-207: LGTM!The fake implementation correctly returns nil, allowing tests to proceed without actual registry initialization.
pkg/helm/actions/get_registry_test.go (2)
41-45: LGTM!The test correctly verifies that passing a nil configuration returns an appropriate error message.
47-52: LGTM!The test appropriately validates that
GetDefaultOCIRegistryworks with a minimal configuration containing only the required fields.pkg/helm/handlers/handlers.go (8)
25-52: LGTM!The initialization of
getDefaultOCIRegistrytoactions.GetDefaultOCIRegistryin theNewfunction follows the established pattern for wiring action functions and correctly integrates the new OCI registry support.
54-77: LGTM!The addition of the
getDefaultOCIRegistryfield to thehelmHandlersstruct with the correct signature enables dependency injection for testing and follows the established pattern.
122-161: LGTM!The integration of
getDefaultOCIRegistryinHandleHelmInstallis correctly placed after configuration creation and before REST config initialization. The error handling with 502 status code is appropriate for registry client initialization failures.
163-199: LGTM!The integration of
getDefaultOCIRegistryinHandleHelmInstallAsyncfollows the same correct pattern asHandleHelmInstall, ensuring consistent registry client initialization across async operations.
248-285: LGTM!The integration of
getDefaultOCIRegistryinHandleChartGetcorrectly ensures OCI registry support for chart retrieval operations, with appropriate error handling.
287-326: LGTM!The integration of
getDefaultOCIRegistryinHandleUpgradeReleaseensures OCI chart support for upgrade operations, with consistent error handling.
328-364: LGTM!The integration of
getDefaultOCIRegistryinHandleUpgradeReleaseAsyncmaintains consistency with the synchronous upgrade handler and ensures OCI support for async operations.
386-414: LGTM!The integration of
getDefaultOCIRegistryinHandleRollbackReleaseensures OCI support for rollback operations, maintaining consistency with other mutating handlers.
|
/retest |
|
(Don't mind me...I'm just learning Prow....) /kind bug |
|
@webbnh: Can not set label jira/valid-bug: Must be member in one of these teams: [openshift-patch-managers openshift-staff-engineers openshift-release-oversight openshift-sustaining-engineers] DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@webbnh: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
Looks good to me, although I didn't dive all the way into the effects of this change on the upstream code. I have a couple of small comments for your consideration.
I'm looking forward to seeing what this change looks like after the refactoring! 🙂
| func TestGetDefaultOCIRegistry_MinimumConfig(t *testing.T) { | ||
| conf := &action.Configuration{} | ||
| err := GetDefaultOCIRegistry(conf) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, conf.RegistryClient, "Registry Client should not be nil") | ||
| } |
There was a problem hiding this comment.
This test looks redundant -- it duplicates lines 30-32 -- the only difference is that it exercises the call with a zero-value action.Configuration instead of the more fully-initialized one instantiated at line 17. Is there any reason not to omit this test?
On the other hand, we don't seem to have a test which exercises the path resulting from an error returned by registry.NewClient(). (I assume that that would require a mock, and I'm not up on how to do that in Go, so I'm not sure what to suggest, here.)
pkg/helm/actions/get_registry.go
Outdated
| if conf == nil { | ||
| return fmt.Errorf("action configuration cannot be nil") | ||
| } | ||
| registryclient, err := registry.NewClient(registry.ClientOptDebug(false)) |
There was a problem hiding this comment.
I think that, under Go's naming conventions, this should be registryClient (with a capital C, as in "camelCase").
|
/jira refresh |
|
@webbnh: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sowmya-sl: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@sowmya-sl, this PR should probably be changed to a draft, since the intention is to base it on #15897 (and, apparently, #15863), right? Also, the test run here shows a problem in #15863: It seems that you need to install /remove-approve |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
component/core
sowmya-sl
force-pushed
the
add-oci-support
branch
from
9f23aec to
09af84b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sowmya-sl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
9 participants
OCI-based Helm charts were failing to install because the action configuration lacked a registry client. This change:
Without a registry client, operations on OCI charts (oci://) would fail with errors about missing registry support.
Fixes: HELM-611
Steps to test: