Move resource verification into a separate step if argo rollouts support feature is enabled#2002
Move resource verification into a separate step if argo rollouts support feature is enabled#2002akavalchuk wants to merge 3 commits into
Conversation
scme0
left a comment
scme0
left a comment
There was a problem hiding this comment.
Just a few comments. One important one. Make sure you remove the guard in ResourceStatusReportScriptWrapper.cs before you merge
| var waitForJobs = variables.GetFlag(SpecialVariables.WaitForJobs); | ||
|
|
||
| var statusCheck = statusReporter.Start(timeoutSeconds, waitForJobs, resources); | ||
| var success = statusCheck.WaitForCompletionOrTimeout(CancellationToken.None).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Sucks that Command doesn't handle async natively. We can probably refactor this in Calamari now that we aren't support .net framework. Maybe I'll look at it for sharpening.
|
|
||
| if (!success) | ||
| { | ||
| throw new CommandException("Resource verification failed."); |
There was a problem hiding this comment.
I can't remember, do we get logging for the failure in statusReporter?
There was a problem hiding this comment.
Yes, it logs that status check terminated because of errors. Do you suggest removing log from here?
There was a problem hiding this comment.
Anything in verbose? Might be good to know which resource failed?
There was a problem hiding this comment.
Yes, verbose logs have info about what exactly failed
| continue; | ||
|
|
||
| var name = string.IsNullOrEmpty(resource.Namespace) ? resource.Name : $"{resource.Namespace}/{resource.Name}"; | ||
| log.Warn($"Unable to fully verify resource '{gvk}' '{name}'. Calamari does not know the readiness criteria for this resource type; only its existence will be confirmed."); |
There was a problem hiding this comment.
Maybe one for Idan, but do we want to indicate that using the Agent with kubernetes monitor may unlock verification for their type. Possibly a link to a public doc which will indicate which types are supported for verification? It should be pretty expansive once https://linear.app/octopus/issue/SIE-143/add-argo-rollout-support-to-kubernetes-monitor is completed.
| } | ||
| conventions.Add(new KubernetesAuthContextConvention(log, commandLineRunner, kubectl, fileSystem)); | ||
|
|
||
| new ConventionProcessor(deployment, conventions, log).RunConventions(); |
There was a problem hiding this comment.
Is there no shared code we can leverage for auth stuff?
| { | ||
| Options.Parse(commandLineArguments); | ||
|
|
||
| var json = variables.Get(SpecialVariables.AppliedResources); |
There was a problem hiding this comment.
If the variable doesn't exist we should fail, I think. Otherwise people might think verification is working correctly when there is actually some issue with it.
There was a problem hiding this comment.
If the variable exists and it's an empty array [], then the info log is good.
| if (OctopusFeatureToggles.ArgoRolloutsSupportFeatureToggle.IsEnabled(variables)) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
We shouldn't do this block yet because not all kubernetes steps are supported for the new verification step. This guard should be added to SIE-155 - see the ticket for details.
There was a problem hiding this comment.
Currently only the steps with dedicated commands in Calamari are supported: RawYaml, Helm, Kustomize
akavalchuk
force-pushed
the
akavalchuk/sie-63-extract-verify-resources-action
branch
2 times, most recently
from
673c4b1 to
d8da1f9
Compare
akavalchuk
force-pushed
the
akavalchuk/sie-63-extract-verify-resources-action
branch
from
d8da1f9 to
40da78c
Compare
2 participants
Adds a new kubernetes-verify-resources Calamari command and gates inline resource status checking behind the ArgoRolloutsSupport feature toggle. When the toggle is enabled, deploy steps apply resources and emit the AppliedResources output variable, but defer status verification to a separate verification action that runs afterwards.
Relates to SIE-63
Related Server PR