feat(v3): install Helm charts directly without KOTS CLI #2715
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| func WithKubeConfigPath(path string) AppInstallManagerOption { |
There was a problem hiding this comment.
good catch. i think i'll need your work on stopping deploys from kots to test this properly e2e
| return len(p), nil | ||
| } | ||
|
|
||
| func (m *appInstallManager) setupHelmClient() error { |
There was a problem hiding this comment.
why not make hcli required? also isnt it better to initialize things in the constructor as errors will happen unexpectedly late?
There was a problem hiding this comment.
but the cluster won't be up yet during initialization in the linux target scenario. hcli is mainly overridden by tests
| logFn := m.logFn("app-helm") | ||
|
|
||
| if m.releaseData == nil || len(m.releaseData.HelmChartArchives) == 0 { | ||
| logFn("no helm charts found in release data") |
There was a problem hiding this comment.
is this an error? what is a use case for not having any helm charts?
| for i, chartArchive := range m.releaseData.HelmChartArchives { | ||
| logFn("installing chart %d/%d", i+1, len(m.releaseData.HelmChartArchives)) | ||
|
|
||
| if err := m.installHelmChart(ctx, chartArchive, i); err != nil { |
There was a problem hiding this comment.
rather than pass the index here as an argument, can you wrap the error in the index?
| defer tmpFile.Close() | ||
|
|
||
| if _, err := tmpFile.Write(chartArchive); err != nil { | ||
| os.Remove(tmpFile.Name()) |
There was a problem hiding this comment.
better to acknowledge you are ignoring an error here
| os.Remove(tmpFile.Name()) | |
| _ = os.Remove(tmpFile.Name()) |
| } | ||
|
|
||
| logFn("successfully installed chart %d/%d", i+1, len(m.releaseData.HelmChartArchives)) | ||
| } |
There was a problem hiding this comment.
Bug: Helm Chart Absence Causes Installation Failure
The application installation fails if no Helm charts are found in the release data. The installHelmCharts function currently errors out in this scenario, but some applications legitimately don't include Helm charts. This prevents the KOTS CLI setup and other necessary installation steps from completing, even when no charts are expected.
sgalsaleh
deleted the
salah/sc-128045/update-app-install-endpoint-to-install-helm
branch
This reverts commit 9f1cb9f.
* Revert "feat(v3): pass isEmbeddedClusterV3 to adminconsole chart (#2723)" This reverts commit 58fb99e. * Revert "Extract installable charts test improvements (#2722)" This reverts commit 6aee665. * Revert "feat(v3): use values and optionalValues from HelmChart custom resource when installing app charts (#2719)" This reverts commit ed1fceb. * Revert "feat(v3): install Helm charts directly without KOTS CLI (#2715)" This reverts commit 9f1cb9f.
* feat(v3): install Helm charts directly without KOTS CLI * gofmt * fix tests * gofmt * update docs * feedback
What this PR does / why we need it:
Installs Helm charts directly without relying on the KOTS CLI
Which issue(s) this PR fixes:
SC-128045
Does this PR require a test?
Yes
Does this PR require a release note?
Does this PR require documentation?
NONE