Report helm manifests for live status #1368
Conversation
| return this; | ||
| } | ||
|
|
||
| public HelmCli WithExecutable(IVariables variables) |
There was a problem hiding this comment.
Same logic used in HelmUpgradeConvention to get the executable - now with tests!
The only thing that the existing convention does is a chmod +x in the script before running it - I'm going to do some testing to make sure that the CommandLineRunner doesn't need that to happen before I merge this.
There was a problem hiding this comment.
I've tested this on linux and it works as well as it does without - there are some permission issues with the existing step if you use helm from a package and it's got the wrong permissions.
I'll ask someone to test it out in windows.
There was a problem hiding this comment.
Successfully tested on a windows machine with a windows worker: https://octopusdeploy.slack.com/archives/C0431AXP230/p1730072949591379
| public class CaptureCommandOutput : ICommandInvocationOutputSink, ICommandOutput | ||
| { | ||
| private readonly List<Message> messages = new List<Message>(); | ||
| public Message[] Messages => messages.ToArray(); | ||
|
|
||
| public IEnumerable<string> InfoLogs => Messages.Where(m => m.Level == Level.Info).Select(m => m.Text).ToArray(); | ||
|
|
||
| public string MergeInfoLogs() => string.Join(Environment.NewLine, InfoLogs); |
There was a problem hiding this comment.
What are your thoughts on having the newline character depend on the OS where Calamari is running?
There was a problem hiding this comment.
I don't have a lot of them - there are existing examples of joining these log messages with Environment.NewLine so my assumption is that it works fine in all cases we're using it at the moment.
Sticking with Unix newlines might be a good idea to be consistent - perhaps there is a bug we just haven't seen come up yet?
By the same token, perhaps there is no bug because windows people tend to have everything on windows so we're just getting lucky...
I don't have any good answers aside from sticking with the status quo
| return this; | ||
| } | ||
|
|
||
| if (variables.GetIndexes(PackageVariables.PackageCollection) |
There was a problem hiding this comment.
Any other variables we need to handle? Or is this all?
There was a problem hiding this comment.
The variables being passed here are all the variables that were passed down in the deployment, which I believe should be the lot of them.
This is eq to what we are currently doing for the helm upgrade step
Sends manifests for helm installations back to server after the helm installation is completed.
This is the first pass at implementing helm support, but isn't yet ideal because we won't get any information back during deployments if
--waitor--atomicflags are used. This is known and we will be coming back to solve for this.I have added a
HelmCliclass to capture the logic used for helm (similar to what we have done with other CLI tools). For now this is purely forhelm get manifestbut there is no reason that it shouldn't be used forhelm upgradeonce we've proven it out. I opted not to do any actual refactoring here for the sake of time, but this is a step in the right direction imo.There are so many ways of running things in Calamari (eg. the existing helm convention uses the
ScriptEngine, is there a reason for that?) so I'm hoping that creating aHelmCliis the right approach. - (28-10-24) Discussed this with Rob and we think it's a decent approach