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

add new separate terraform plans for staging and production #309

Merged
merged 18 commits into from
Nov 1, 2021

Conversation

sastels
Copy link
Contributor

@sastels sastels commented Oct 29, 2021

Summary | Résumé

Separate the terraform plan for staging and production. Use the new action for the plan to better surface results.

Note that changes to the terraform / terragrunt files were to fix formatting issues surfaced by terraform fmt
(with the exception of the variable removed in aws/rds/variables.tf

Test instructions | Instructions pour tester la modification

Help requested | Aide requise

Unresolved questions / Out of scope | Questions non résolues ou hors sujet

Reviewer checklist | Liste de vérification du réviseur

This is a suggested checklist of questions reviewers might ask during their
review | Voici une suggestion de liste de vérification comprenant des questions
que les réviseurs pourraient poser pendant leur examen :

  • Does this meet a user need? | Est-ce que ça répond à un besoin utilisateur?
  • Is it accessible? | Est-ce que c’est accessible?
  • Is it translated between both offical languages? | Est-ce dans les deux
    langues officielles?
  • Is the code maintainable? | Est-ce que le code peut être maintenu?
  • Have you tested it? | L’avez-vous testé?
  • Are there automated tests? | Y a-t-il des tests automatisés?
  • Does this cause automated test coverage to drop? | Est-ce que ça entraîne
    une baisse de la quantité de code couvert par les tests automatisés?
  • Does this break existing functionality? | Est-ce que ça brise une
    fonctionnalité existante?
  • Should this be split into smaller PRs to decrease change risk? | Est-ce
    que ça devrait être divisé en de plus petites demandes de tirage (« pull
    requests ») afin de réduire le risque lié aux modifications?
  • Does this change the privacy policy? | Est-ce que ça entraîne une
    modification de la politique de confidentialité?
  • Does this introduce any security concerns? | Est-ce que ça introduit des
    préoccupations liées à la sécurité?
  • Does this significantly alter performance? | Est-ce que ça modifie de
    façon importante la performance?
  • What is the risk level of using added dependencies? | Quel est le degré de
    risque d’utiliser des dépendances ajoutées?
  • Should any documentation be updated as a result of this? (i.e. README
    setup, etc.) | Faudra-t-il mettre à jour la documentation à la suite de ce
    changement (fichier README, etc.)?

@sastels sastels force-pushed the new-terraform-plan-action branch from 69ccb3a to 6e0479f Compare October 29, 2021 15:07
@sastels sastels force-pushed the new-terraform-plan-action branch from 88121cb to c3fd7d4 Compare October 29, 2021 15:11
@github-actions
Copy link

Production: rds

❌   Terraform Format: failed
❌   Terraform Plan: failed

⚠️   Format: run terraform fmt to fix the following:

rds.tf
rds_proxy.tf
variables.tf
Show plan
Warning: Interpolation-only expressions are deprecated

  on rds_proxy.tf line 62, in module "rds_proxy":
  62:     "${local.db_user}" = {

Terraform 0.11 and earlier required all non-constant expressions to be
provided via interpolation syntax, but this pattern is now deprecated. To
silence this warning, remove the "${ sequence from the start and the }"
sequence from the end of this expression, leaving just the inner expression.

Template interpolation syntax is still used to construct strings from
expressions when the template includes multiple interpolation sequences or a
mixture of literal strings and interpolations. This deprecation applies only
to templates that consist entirely of a single interpolation sequence.


Error: No value for required variable

  on variables.tf line 9:
   9: variable "rds_server_db_user" {

The root module input variable "rds_server_db_user" is not set, and has no
default value. Use a -var or -var-file command line argument to provide a
value for this variable.

time=2021-10-29T17:21:00Z level=error msg=1 error occurred:
	* exit status 1


Comment on lines -9 to -12
variable "rds_server_db_user" {
type = string
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable is not used in the code. It was added in #282 where it was briefly used, but in the end the code that used it was deleted from the PR in 8b36b21 (it was in the secrets.tf file that was deleted in that commit)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for cleaning it up.

@sastels
Copy link
Contributor Author

sastels commented Oct 29, 2021

Production: rds

❌   Terraform Format: failed **
❌   Terraform Plan: failed

⚠️   Format: run terraform fmt to fix the following:

rds.tf
rds_proxy.tf
variables.tf

Note that this is failing for production (but not staging) because the production terragrunt plan is using the version of code specified by INFRASTRUCTURE_VERSION.txt, so does not have the formatting fixes contained in this PR.

@sastels sastels force-pushed the new-terraform-plan-action branch from 7ce0241 to 33cc1be Compare October 29, 2021 18:29
@sastels sastels marked this pull request as ready for review October 29, 2021 18:48
@sastels sastels requested a review from jimleroyer as a code owner October 29, 2021 18:48
@sastels
Copy link
Contributor Author

sastels commented Oct 29, 2021

huh maybe I shouldn't be running terragrunt plan in common, @patheard ?

Note that pull_request.yml also reports changing the timestamps.

@sastels sastels requested a review from patheard October 29, 2021 18:49
Comment on lines 6 to 8
- "aws/**"
- "env/production/**"
- "env/terragrunt.hcl"
Copy link
Member

Choose a reason for hiding this comment

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

To cut down on PR noise, it might make sense to set this to the same triggers as the merge_to_main_production.yml paths:
https://github.com/cds-snc/notification-terraform/blob/main/.github/workflows/merge_to_main_production.yml#L8-L10

That way you only see Prod plan comments if you're expecting to have terraform apply run against Prod when you merge.

Copy link
Member

@patheard patheard left a comment

Choose a reason for hiding this comment

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

Looks really good Steve! Just had a minor comment for the production plan path triggers, but it's a preference thing.

@patheard
Copy link
Member

huh maybe I shouldn't be running terragrunt plan in common, @patheard ?

Based on your infra, I think it makes sense to run terragrunt plan in common. The recreation of that Slack Lambda null_resource is unfortunately buried in the module you're using, so it's not something we can ignore.

That being said, we have our own Notify Slack module without this behaviour that might do what you need:
https://github.com/cds-snc/terraform-modules/tree/main/notify_slack

Copy link
Member

@jimleroyer jimleroyer left a comment

Choose a reason for hiding this comment

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

:shipit: (on Monday!)

@jimleroyer
Copy link
Member

Do we know why Terraform still complain about the Slack modules in his plans?

@CalvinRodo
Copy link
Member

Do we know why Terraform still complain about the Slack modules in his plans?

The timestamp trigger is forcing that resource to get tainted.

@jimleroyer
Copy link
Member

@CalvinRodo Seems like this is linked to the Slack aws notify module and has been addressed:
terraform-aws-modules/terraform-aws-notify-slack#84

I opened a PR to disable automatic overwriting of the Slack lambda:
#310

@sastels
Copy link
Contributor Author

sastels commented Nov 1, 2021

It's still complaining that it's... maybe not going to do anything? This might be as good as it gets unless we replace with SRE's slack integration code.

@github-actions
Copy link

github-actions bot commented Nov 1, 2021

Staging: common

✅   Terraform Format: success
✅   Terraform Plan: success

⚠️   WARNING: resources will be destroyed by this change!

Plan: 3 to add, 0 to change, 3 to destroy
Show plan
Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # module.notify_slack_critical.module.lambda.null_resource.archive[0] must be replaced
-/+ resource "null_resource" "archive" {
      ~ id       = "3986637290744123574" -> (known after apply)
      ~ triggers = { # forces replacement
          ~ "timestamp" = "1635790407125145000" -> "<WARNING: Missing lambda zip artifacts wouldn't be restored>"
            # (1 unchanged element hidden)
        }
    }

  # module.notify_slack_general.module.lambda.null_resource.archive[0] must be replaced
-/+ resource "null_resource" "archive" {
      ~ id       = "4054771690764672551" -> (known after apply)
      ~ triggers = { # forces replacement
          ~ "timestamp" = "1635790404755724000" -> "<WARNING: Missing lambda zip artifacts wouldn't be restored>"
            # (1 unchanged element hidden)
        }
    }

  # module.notify_slack_warning.module.lambda.null_resource.archive[0] must be replaced
-/+ resource "null_resource" "archive" {
      ~ id       = "2275826554682586410" -> (known after apply)
      ~ triggers = { # forces replacement
          ~ "timestamp" = "1635790407230048000" -> "<WARNING: Missing lambda zip artifacts wouldn't be restored>"
            # (1 unchanged element hidden)
        }
    }

Plan: 3 to add, 0 to change, 3 to destroy.

------------------------------------------------------------------------

This plan was saved to: plan.tfplan

To perform exactly these actions, run the following command to apply:
    terraform apply "plan.tfplan"

@sastels sastels merged commit 52f3803 into main Nov 1, 2021
@sastels sastels deleted the new-terraform-plan-action branch November 1, 2021 18:42
@sastels sastels mentioned this pull request Nov 1, 2021
14 tasks
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.

4 participants