Skip to content
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

Allow new Helm upgrade command to be executed via script #1409

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

kevjt
Copy link
Contributor

@kevjt kevjt commented Dec 12, 2024

Part of OctopusDeploy/Issues#9168

The new Helm Upgrade command (introduced as part of the KOS for Helm feature) changed the way Helm was being executed. It was previously being executed via a Bash (or Powershell) script, whereas now the Helm command is being invoked directly with the arguments.

Customers who were using Bash-specific formatting in their arguments, e.g. quotes around literals, have experienced errors in their Helm step as these arguments are no longer being processed by Bash.

This PR introduces a feature-flagged workaround that enables affected customers to continue deploying using the new Helm command by reverting to the script-based deployment method.

[sc-99264]

…helm upgrade command to be executed via bash script
@@ -77,7 +82,6 @@ public void UsesCustomHelmExecutableFromPackage()

var (helm, commandLineRunner, workingDirectory, _) = GetHelmCli(expectedExecutable, new CalamariVariables
{
[SpecialVariables.Helm.Packages.CustomHelmExePackageKey] = expectedPackageKey,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find where this variable was being used. The current tests pass even without this here.

{
if (isCustomExecutable)
{
commandBuilder.Add(syntax == ScriptSyntax.PowerShell ? ". " : $"chmod +x \"{ExecutableLocation}\";");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevjt kevjt requested a review from APErebus December 12, 2024 22:45
Copy link
Contributor

@APErebus APErebus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, look good to me. @zentron might have an opinion on the warning message

source/Calamari/Kubernetes/Integration/HelmCli.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@zentron zentron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment around the warning message, otherwise 👍
Thanks for going over this one again.

@kevjt kevjt enabled auto-merge (squash) December 13, 2024 00:58
@kevjt kevjt merged commit a78182f into main Dec 13, 2024
41 checks passed
@kevjt kevjt deleted the kevjt/fix-helm-additional-parameters-regression branch December 13, 2024 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants