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

Deploy template error handling cleanup #296

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Conversation

JenGoldstrich
Copy link
Contributor

@JenGoldstrich JenGoldstrich commented Apr 21, 2023

Currently step_deploy_template is used for deploying KeyVaults, and Virtual Machines, however on CleanUp of this step we try and find a VM either way, and then return an "error" if we don't find the VM, this leads to every build falsely showing that we failed to remove the temporary disk

This PR breaks this logic up, and only tries to search for a VM disk when we're actually in a VM deploy

Partially addresses #190

@@ -299,9 +309,9 @@ func (s *StepDeployTemplate) deleteDeploymentResources(ctx context.Context, depl
resourceName,
resourceGroupName)
if err != nil {
s.say(fmt.Sprintf("Error deleting resource. Will retry.\n"+
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We regularly fail to delete attached network resources related to the resource being still attached, this leads to a lot of spam in the output, here I've changed this to only log the error when we fail for real, since almost every build I run fails this check at least once for at least two different resources.

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Hi there overall this looks good. I made a few comments on the boolean not being immediately clear. Take a look at the alternative suggestions and let me know what you think.

I do suggest inverting the condition in the cleanup step to key if it is a keyVault deployment and return early as opposed to using the if/else statement.

builder/azure/arm/builder.go Outdated Show resolved Hide resolved
builder/azure/arm/builder.go Outdated Show resolved Hide resolved
builder/azure/arm/step_deploy_template.go Outdated Show resolved Hide resolved
builder/azure/arm/step_deploy_template_test.go Outdated Show resolved Hide resolved
@@ -184,6 +187,24 @@ func TestStepDeployTemplateCleanupShouldVHDOSImageInTemporaryResourceGroup(t *te
}
}

func TestStepDeployTemplateCleanupShouldNotDeleteDiskForKeyVaultDeployments(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test

@nywilken
Copy link
Contributor

Not related to your PR but it looks like the Azure plugin needs to have it's partials regenerated

~>  gd
diff --git a/docs-partials/builder/azure/arm/Config-not-required.mdx b/docs-partials/builder/azure/arm/Config-not-required.mdx
index 5d52106..1f7c73c 100644
--- a/docs-partials/builder/azure/arm/Config-not-required.mdx
+++ b/docs-partials/builder/azure/arm/Config-not-required.mdx
@@ -358,7 +358,7 @@
   [Windows](https://learn.microsoft.com/en-us/azure/virtual-machines/windows/hybrid-use-benefit-licensing)
   or
   [Linux](https://learn.microsoft.com/en-us/azure/virtual-machines/linux/azure-hybrid-benefit-linux)
-
+
 - `secure_boot_enabled` (bool) - Specifies if Secure Boot and Trusted Launch is enabled for the Virtual Machine.

 - `vtpm_enabled` (bool) - Specifies if vTPM (virtual Trusted Platform Module) and Trusted Launch is enabled for the Virtual Machine.

@JenGoldstrich JenGoldstrich requested a review from nywilken April 25, 2023 00:43
@JenGoldstrich JenGoldstrich force-pushed the deploy-cleanup-cleanup branch from 750b319 to 6803fea Compare April 25, 2023 00:44
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Looks great! 🚢

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.

2 participants