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

Provide a knob to optionally skip creating the VPC related resources #330

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

yegle
Copy link
Collaborator

@yegle yegle commented May 31, 2024

Description

Provide a var.external_vpc to specify the VPC settings that are not managed by the Terraform configs.

Checklist

General

  • Added the correct label
  • Assigned to a specific person or civiform/deployment-system
  • Created tests which fail without the change (if possible)
  • Performed manual testing (at a minimum run bin/setup without your changes and then bin/deploy with your changes to ensure your changes don't break existing deployments)
  • Extended the README / documentation, if necessary

Instructions for manual testing

Tested by patching this change to the checkout folder against my dev environment, w/o vars.external_vpc.

(NOTE: it will be difficult to test w/ vars.external_vpc, as Terraform will try to destroy some resources)

Issue(s) this completes

Fixes civiform/civiform#7378

This reverts commit ed78f28. It also fixed the breakage originally introduced in f1e3565, where a local variable was referenced without the local. prefix.

yegle added 2 commits May 31, 2024 10:25
…lated resources" (#328)"

This reverts commit ed78f28.

This also fixed the breakage originally introduced in f1e3565
@dkatzz
Copy link
Contributor

dkatzz commented Jun 3, 2024

Thanks for fixing this, @yegle! Would you mind updating the title to remove the "revert" parts. And then in the description could you include a bit more information about what changed and also include the github issue it relates to (similar to what was included in the original PR, but with some information about what changed between that PR and this one to fix the issue)? And could you also include in the description how you tested it?

@yegle yegle changed the title Revert "Revert "Provide a knob to optionally skip creating the VPC re… Provide a knob to optionally skip creating the VPC related resources Jun 3, 2024
@yegle
Copy link
Collaborator Author

yegle commented Jun 3, 2024

PR description updated, PTAL.

@dkatzz
Copy link
Contributor

dkatzz commented Jun 3, 2024

Could we try testing with vars.external_vpc set for a bin/setup? In that case there won't be existing resources. You can first nuke the resources in the environment or use a different app-prefix.

@dkatzz dkatzz requested review from dkatzz and nb1701 June 3, 2024 17:07
@yegle
Copy link
Collaborator Author

yegle commented Jun 6, 2024

With the moved block added in the last commit, I did the following test:

Testing w/o external vpc configs:

  1. Deploy to my dev project using the main branch
  2. Switch to the fix-vpc-knob branch (this PR), run terraform plan

Output:

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the
following symbols:
  ~ update in-place
-/+ destroy and then create replacement
...
Plan: 2 to add, 1 to change, 2 to destroy.

(resource.aws_ecs_task_definition.civiform_only and resource.aws_ecs_task_definition.civiform_with_monitoring were force replaced due to the image version change)

I did not test w/ external vpc configs. We will need to work with the CE on this, since if the CE partially sets up the VPC using the Terraform config and then sets the external_vpc variable, it will destroy all vpc related resources, even if you want to reuse the partially created resource.

When the CE decides to use this knob, we should work closely with them, run terraform state rm for every resource that is proposed to destroy and then switch to the external managed VPC resources.

SGTY?

@dkatzz
Copy link
Contributor

dkatzz commented Jun 7, 2024

With the moved block added in the last commit, I did the following test:

Testing w/o external vpc configs:

  1. Deploy to my dev project using the main branch
  2. Switch to the fix-vpc-knob branch (this PR), run terraform plan

Output:

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the
following symbols:
  ~ update in-place
-/+ destroy and then create replacement
...
Plan: 2 to add, 1 to change, 2 to destroy.

(resource.aws_ecs_task_definition.civiform_only and resource.aws_ecs_task_definition.civiform_with_monitoring were force replaced due to the image version change)

I did not test w/ external vpc configs. We will need to work with the CE on this, since if the CE partially sets up the VPC using the Terraform config and then sets the external_vpc variable, it will destroy all vpc related resources, even if you want to reuse the partially created resource.

When the CE decides to use this knob, we should work closely with them, run terraform state rm for every resource that is proposed to destroy and then switch to the external managed VPC resources.

SGTY?

We'll actually be using a different AWS account for them for this deployment than their previous deployment, so we shouldn't have to do the terraform state rm. I think we should be able to just add the new VPC information before the deployment and then see how that goes.

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.

Allow custom resources to be used for a CiviForm deployment
2 participants