-
Notifications
You must be signed in to change notification settings - Fork 72
[Feature] [Platform] Installer move to OCI #1987
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
Conversation
76ccda4 to
1f6d4ab
Compare
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 migrates the platform installer from using an S3-based chart registry to an OCI (Open Container Initiative) registry approach. The key changes consolidate registry client functionality, add environment variable support for license manager credentials, and introduce a stage-based registry host naming convention.
- Migrated from S3-based chart repository to OCI registry for Helm chart distribution
- Consolidated registry client creation into a reusable
cli.Registrymodule - Added environment variable support for license manager credentials
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/k8sutil/helm/package.go | Added Stage field to PackageSpec and GetStage() method with "prd" default |
| pkg/util/executor/executor.go | Added default case with continue in Timeout select statement |
| pkg/util/cli/registry.go | New file implementing registry client factory with configuration support |
| pkg/util/cli/lm.go | Added RegistryHosts method and EnvEnabled support for credentials |
| pkg/util/cli/flag.go | Implemented environment variable loading for flags via GetFromEnv() |
| pkg/platform/runner.go | Replaced flagPlatformEndpoint with flagRegistry in runner |
| pkg/platform/regclient.go | Removed - functionality moved to cli.Registry |
| pkg/platform/package_registry.go | Removed - registry redirection command removed |
| pkg/platform/package_install.go | Updated to use OCI registry, improved logging with structured logger |
| pkg/platform/package_import.go | Updated to use consolidated registry client |
| pkg/platform/package_export.go | Updated to use OCI registry with endpoint-based chart references |
| pkg/platform/package.go | Removed packageRegistry command |
| pkg/platform/pack/utils.go | New file with OCI chart reference and export utilities |
| pkg/platform/pack/export.go | Updated to use OCI registry instead of ChartManager |
| pkg/platform/flags.go | Replaced individual registry flags with cli.Registry |
| pkg/platform/chart_manager.go | Removed - S3-based chart manager no longer needed |
| docs/cli/arangodb_operator_platform.md | Updated documentation to reflect new flags and removed commands |
| CHANGELOG.md | Added bugfix entry for installer OCI migration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1f6d4ab to
9d6b685
Compare
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
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if errors.Is(err, io.EOF) { | ||
| return errors.Errorf("Chart %s is not ready", name) | ||
| } | ||
|
|
||
| return err |
Copilot
AI
Nov 11, 2025
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.
The error logic is inverted. When err is io.EOF, it indicates success (the timeout function returns io.EOF to signal completion at line 214 and 309), but here it's treated as a failure. The check should be !errors.Is(err, io.EOF) instead, or the logic should return success when io.EOF is encountered.
| if errors.Is(err, io.EOF) { | |
| return errors.Errorf("Chart %s is not ready", name) | |
| } | |
| return err | |
| if !errors.Is(err, io.EOF) { | |
| return err | |
| } | |
| // Chart is ready (io.EOF signals success) |
| if err := blob.Close(); err != nil { | ||
| return err | ||
| } |
Copilot
AI
Nov 11, 2025
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.
The blob is being closed twice: once via the defer statement at line 333 and once explicitly here. This will cause an error on the second close attempt. Remove either the defer statement or this explicit close call.
pkg/platform/pack/export.go
Outdated
| "context" | ||
| "fmt" | ||
| "io" | ||
| "io/ioutil" |
Copilot
AI
Nov 11, 2025
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.
The io/ioutil package has been deprecated since Go 1.16. Use io package instead. The functions have been moved to io and os packages.
| "io/ioutil" |
No description provided.