Add AppliedResources output variable#1947
Conversation
|
|
scme0
left a comment
scme0
left a comment
There was a problem hiding this comment.
I think this is looking good and I've pre-approved. 🚀 But please take a look at the comments and let me know if something doesn't make sense.
|
|
||
| var json = JsonConvert.SerializeObject(resourceList); | ||
|
|
||
| log.SetOutputVariable("AppliedResources", json, deployment.Variables); |
There was a problem hiding this comment.
Is there a place we can set this as a const? I think we might do it for other output variables?
| } | ||
| catch (Exception ex) | ||
| { | ||
| log.Warn($"Failed to set applied resources output variable: {ex.Message}"); |
There was a problem hiding this comment.
What do we expect to fail here? Just getting the manifest or getting the resources from manifest?
| } | ||
|
|
||
| [Test] | ||
| public async Task DoesNotSetAppliedResourcesOutputVariable_WhenFeatureToggleIsDisabled() |
There was a problem hiding this comment.
I guess it doesn't really hurt but because the feature toggle guard is in the SetAppliedResourcesOutputVariable method, we don't really need this test as well?
| } | ||
|
|
||
| [Test] | ||
| public void DoesNotCallGetManifest_WhenFeatureToggleIsDisabled() |
There was a problem hiding this comment.
This test is basically testing one part of the test above DoesNotSetAppliedResourcesOutputVariable_WhenFeatureToggleIsDisabled() - maybe we can just remove this one?
| } | ||
|
|
||
| [Test] | ||
| public void CallsGetManifest_WhenFeatureToggleIsEnabled() |
There was a problem hiding this comment.
Similar to my comment above, should we just add the assertion into the SetsAppliedResourcesOutputVariable_WhenFeatureToggleIsEnabled test?
| } | ||
|
|
||
| [Test] | ||
| public void LogsWarningAndContinues_WhenGetManifestFails() |
There was a problem hiding this comment.
I wonder if we should fail on this - The verification step will probably fail if it doesn't get a valid output variable value.
3 participants
Adds output variable with applied resources so it would be possible to move verification into a separate step.
For kubernates steps we've already had identifiers, so it's only a matter of setting the variable.
For helm step identifiers received be calling
helm get manifest <release> --revision <N>after the upgrade completes.