feat: implement --push-repository flag #94
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import ( | |
| "os" | ||
| "path/filepath" | ||
| "regexp" | ||
| "strings" | ||
|
|
||
| "github.com/spf13/cobra" | ||
| "github.com/vmware-labs/distribution-tooling-for-helm/cmd/dt/config" | ||
|
|
@@ -18,10 +19,8 @@ import ( | |
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/chartutils" | ||
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/imagelock" | ||
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/log" | ||
|
There was a problem hiding this comment. TODO: undo reordering |
||
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/log/logrus" | ||
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/log/silent" | ||
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/relocator" | ||
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/utils" | ||
| "github.com/vmware-labs/distribution-tooling-for-helm/pkg/wrapping" | ||
|
|
@@ -238,8 +237,8 @@ func NewConfig(opts ...Option) *Config { | |
| } | ||
|
|
||
| // Chart unwraps a Helm chart | ||
| func Chart(inputChart, registryURL, pushRepository string, pushChartURL string, opts ...Option) (string, *chartutils.Chart, error) { | ||
| return unwrapChart(inputChart, registryURL, pushRepository, pushChartURL, opts...) | ||
| } | ||
|
|
||
| func askYesNoQuestion(msg string, cfg *Config) bool { | ||
|
|
@@ -252,19 +251,20 @@ func askYesNoQuestion(msg string, cfg *Config) bool { | |
| return widgets.ShowYesNoQuestion(msg) | ||
| } | ||
|
|
||
| func unwrapChart(inputChart, registryURL, pushRepository string, pushChartURL string, opts ...Option) (string, *chartutils.Chart, error) { | ||
|
|
||
| cfg := NewConfig(opts...) | ||
|
|
||
| ctx := cfg.Context | ||
| parentLog := cfg.GetLogger() | ||
|
|
||
| if registryURL == "" { | ||
| return "", nil, fmt.Errorf("the registry cannot be empty") | ||
| } | ||
|
|
||
| tempDir, err := cfg.GetTemporaryDirectory() | ||
| if err != nil { | ||
| return "", nil, fmt.Errorf("failed to create temporary directory: %w", err) | ||
| } | ||
|
|
||
| l := parentLog.StartSection(fmt.Sprintf("Unwrapping Helm chart %q", inputChart)) | ||
|
|
@@ -284,12 +284,12 @@ func unwrapChart(inputChart, registryURL, pushChartURL string, opts ...Option) ( | |
| ), | ||
| ) | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
|
|
||
| wrap, err := wrapping.Load(chartPath) | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
| if err := l.ExecuteStep(fmt.Sprintf("Relocating %q with prefix %q", wrap.ChartDir(), registryURL), func() error { | ||
| return relocator.RelocateChartDir( | ||
|
|
@@ -298,7 +298,7 @@ func unwrapChart(inputChart, registryURL, pushChartURL string, opts ...Option) ( | |
| relocator.WithSkipImageRelocation(cfg.SkipImageRelocation), | ||
| ) | ||
| }); err != nil { | ||
| return "", nil, l.Failf("failed to relocate %q: %w", chartPath, err) | ||
| } | ||
| l.Infof("Helm chart relocated successfully") | ||
|
|
||
|
|
@@ -311,23 +311,27 @@ func unwrapChart(inputChart, registryURL, pushChartURL string, opts ...Option) ( | |
| } | ||
| if askYesNoQuestion(l.PrefixText("Do you want to push the wrapped images to the OCI registry?"), cfg) { | ||
| if err := l.Section("Pushing Images", func(subLog log.SectionLogger) error { | ||
| return pushChartImagesAndVerify(ctx, wrap, registryURL, pushRepository, NewConfig(append(opts, WithLogger(subLog))...)) | ||
| }); err != nil { | ||
| return "", nil, l.Failf("Failed to push images: %w", err) | ||
| } | ||
| l.Printf(widgets.TerminalSpacer) | ||
| } | ||
| } | ||
|
|
||
| if askYesNoQuestion(l.PrefixText("Do you want to push the Helm chart to the OCI registry?"), cfg) { | ||
|
|
||
| if pushChartURL == "" { | ||
| if pushRepository == "" { | ||
| // we will push the chart to the same registry as the containers | ||
| pushChartURL = registryURL | ||
| } else { | ||
| // we will push the chart to the same registry as defined by pushRepository | ||
| pushChartURL = pushRepository | ||
| } | ||
| cfg.Auth = cfg.ContainerRegistryAuth | ||
| } | ||
| pushChartURL = normalizeOCIURL(pushChartURL) | ||
| if err := l.ExecuteStep(fmt.Sprintf("Pushing Helm chart to %q", pushChartURL), func() error { | ||
| return utils.ExecuteWithRetry(maxRetries, func(try int, prevErr error) error { | ||
| if try > 0 { | ||
|
|
@@ -336,16 +340,24 @@ func unwrapChart(inputChart, registryURL, pushChartURL string, opts ...Option) ( | |
| return pushChart(ctx, wrap, pushChartURL, cfg) | ||
| }) | ||
| }); err != nil { | ||
| return "", nil, l.Failf("Failed to push Helm chart: %w", err) | ||
| } | ||
|
|
||
| l.Infof("Helm chart successfully pushed") | ||
|
|
||
| fullChartURL := fmt.Sprintf("%s/%s", pushChartURL, wrap.Chart().Name()) | ||
|
|
||
| // point chart url to the registryURL if pushRepository is set | ||
| if pushRepository != "" && strings.Contains(pushChartURL, pushRepository) { | ||
| fullChartURL = strings.Replace(fullChartURL, pushRepository, registryURL, 1) | ||
| } | ||
|
|
||
| return fullChartURL, wrap.Chart(), nil | ||
| } | ||
| return "", wrap.Chart(), nil | ||
| } | ||
|
|
||
| func pushChartImagesAndVerify(ctx context.Context, wrap wrapping.Wrap, registryURL string, pushRepository string, cfg *Config) error { | ||
| lockFile := wrap.LockFilePath() | ||
|
|
||
| l := cfg.GetLogger() | ||
|
|
@@ -355,6 +367,8 @@ func pushChartImagesAndVerify(ctx context.Context, wrap wrapping.Wrap, cfg *Conf | |
| if err := push.ChartImages( | ||
| wrap, | ||
| wrap.ImagesDir(), | ||
| registryURL, | ||
| pushRepository, | ||
| chartutils.WithLog(silent.NewLogger()), | ||
| chartutils.WithContext(ctx), | ||
| chartutils.WithArtifactsDir(wrap.ImageArtifactsDir()), | ||
|
|
@@ -454,10 +468,12 @@ func NewCmd(cfg *config.Config) *cobra.Command { | |
| var ( | ||
| sayYes bool | ||
| pushChartURL string | ||
| pushRepository string | ||
| version string | ||
| skipImageRelocation bool | ||
| skipPullImages bool | ||
| ) | ||
|
|
||
| valuesFiles := []string{"values.yaml"} | ||
| cmd := &cobra.Command{ | ||
| Use: "unwrap FILE OCI_URI", | ||
|
|
@@ -480,7 +496,8 @@ func NewCmd(cfg *config.Config) *cobra.Command { | |
| if err != nil { | ||
| return fmt.Errorf("failed to create temporary directory: %v", err) | ||
| } | ||
|
|
||
| fullChartURL, chart, err := unwrapChart(inputChart, registryURL, pushRepository, pushChartURL, | ||
| WithLogger(l), | ||
| WithSayYes(sayYes), | ||
| WithContext(ctx), | ||
|
|
@@ -496,9 +513,11 @@ func NewCmd(cfg *config.Config) *cobra.Command { | |
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var successMessage = "Helm chart unwrapped successfully" | ||
| if fullChartURL != "" { | ||
| //successMessage = fmt.Sprintf(`%s: You can use it now by running "helm install %s --generate-name"`, successMessage, fullChartURL) | ||
|
There was a problem hiding this comment. Somehow the index feature did not work in here as i was not able to pull chart without specifying the version That's mainly the reason for this change. I am not sure why that |
||
| successMessage = fmt.Sprintf(`%s: "helm install %s --generate-name --version %s"`, successMessage, fullChartURL, chart.Version()) | ||
| } | ||
| l.Printf(widgets.TerminalSpacer) | ||
| l.Successf(successMessage) | ||
|
|
@@ -508,6 +527,7 @@ func NewCmd(cfg *config.Config) *cobra.Command { | |
|
|
||
| cmd.PersistentFlags().StringVar(&version, "version", version, "when unwrapping remote Helm charts from OCI, version to request") | ||
| cmd.PersistentFlags().StringVar(&pushChartURL, "push-chart-url", pushChartURL, "push the unwrapped Helm chart to the given URL") | ||
| cmd.PersistentFlags().StringVar(&pushRepository, "push-repository", pushRepository, "the repository to be used instead for write operations e.g (demo.goharbor.io/write/test_repo)") | ||
|
There was a problem hiding this comment. I guess a better parameter name would be There was a problem hiding this comment. Maybe. Unclear to me as the current flag for helm chart is There was a problem hiding this comment. as oci repos are general purpose not specific to store binary images, charts, or just a file just registry might be a good option. Naming is the hardest in here. Let me know what you will like me to change and to what and I will update PR when that's defined. Thanks for having a look There was a problem hiding this comment. I'd go for sth like this: cmd.PersistentFlags().StringVar(&pushContainerRegistry, "push-container-registry", pushContainerRegistry, "push the container images included in the unwrapped Helm chart to the given container registry (including namespace)")What do you think @mpermar? There was a problem hiding this comment. Agree. I will attempt have this clean up tomorrow. Thanks There was a problem hiding this comment. Actually I take it back @machecazzon @juan131. The value can be both a registry and a repository so probably would need some other name like But, actually, while looking in detail to the naming, and this is totally my bad as I am lost touch with this project and it has evolved quite a while since I left it, I can see there is a There was a problem hiding this comment. Have been thinking about this a bit more and I believe that having
That's more like what I did on my first attempt to cover the use case. Right now I have a custom build with what I need to have the desired outputs that includes this and some other changes and is not a big deal but I would prefer that this tool could support this behaviour. Back into the naming, kaniko has this switch and it's format somehow make me think of this Thread. https://github.com/GoogleContainerTools/kaniko?tab=readme-ov-file#flag---registry-map Very likely I will abandon this PR and come back later with a better solution and better PR hopefully, but if wanted can still try to address this PR comments. In regards the
My current scenario: GAR Reader / GAR Writer
For all the previous to work transparently It's crucial to guarantee images path preservation from Gitlab to GAR and or have controlled rewrite if desired as there are some variants and eventual some restrictions depending on the unwrap target ghcr.io | docker.io | registry.gitlab.com | .... There was a problem hiding this comment. That's great context. And wow, you have built a thing there. I'm approaching your PR here from a point of view of trying to keep the tool as simplest as possible. Ideally I'd like to have very simple building blocks even when that might come at the expense of having extra complexity in the client scripts. It makes this project more sustainable in the long run. From that perspective I see that But at the same time I have to be honest here, I'm humbled by your use case and what you are doing. So, I'm not sure if I'm missing anything obvious. Perhaps the right thing to do is as you say to abandon this one and then try to approach it with some simpler baby-step kind of change. Not sure, tbh. There was a problem hiding this comment. Hello @mpermar Sorry for delay getting back. I ended with something like this
|
||
| cmd.PersistentFlags().BoolVar(&sayYes, "yes", sayYes, "respond 'yes' to any yes/no question") | ||
| cmd.PersistentFlags().StringSliceVar(&valuesFiles, "values", valuesFiles, "values files to relocate images (can specify multiple)") | ||
| cmd.PersistentFlags().BoolVar(&skipImageRelocation, "skip-image-relocation", skipImageRelocation, "Skip relocating image references in the different files") | ||
|
|
||
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.
Should we use a new header before explaining this new flag?
Also, I find this sentence confusing. What's "the original" here?
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.
Yes I wanted to refer to
helm dt unwrap oci://original_repo.Perhaps unwrapping target ? Or what do you recommend?
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.
Maybe something like
Unwrapping a Helm chart to a different registry than the one used on relocationorUsing different registries for distribution and relocation?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.
It would also be nice to add some context on why this is needed. Something like:
Sometimes you might need the content on the Helm chart pointing to a registry different than where the Helm chart needs to be pushed to. This is common for example on Airgap environments where a Helm chart might need to be pushed to an interim environment different from where it is meant to be consumed from, or for example in edge distribution channels where the Helm charts might need to be moved from an interim release registry into an edge registry for general public usage.