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

fix: Replace deprecate aws_cloudwatch_event_rule.is_enabled, requires provide upgrade #3655

Merged
merged 20 commits into from
Dec 20, 2023
Merged

fix: Replace deprecate aws_cloudwatch_event_rule.is_enabled, requires provide upgrade #3655

merged 20 commits into from
Dec 20, 2023

Conversation

officel
Copy link
Contributor

@officel officel commented Dec 7, 2023

see #3652

@npalm npalm self-requested a review December 7, 2023 21:25
Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@officel thx, change looks good. Need to run a quick test asap.

@npalm
Copy link
Member

npalm commented Dec 8, 2023

Just ran a deployment got the following error

➜  multi-runner git:(fix/deprecate_aws_cloudwatch_event_rule.is_enabled) ✗ terraform apply                               <aws:swcoe-dev-admin>
╷
│ Error: expected state to be one of ["ENABLED" "DISABLED" "ENABLED_WITH_ALL_CLOUDTRAIL_MANAGEMENT_EVENTS"], got true
│
│   with module.runners.module.ami_housekeeper.aws_cloudwatch_event_rule.ami_housekeeper,
│   on ../../modules/ami-housekeeper/main.tf line 106, in resource "aws_cloudwatch_event_rule" "ami_housekeeper":
│  106:   state               = true

See also https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_event_rule#state

Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

modules/ami-housekeeper/main.tf Outdated Show resolved Hide resolved
officel and others added 4 commits December 9, 2023 05:49
see https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_event_rule#state

At first we forced enable.
but now that `ENABLED_WITH_ALL_CLOUDTRAIL_MANAGEMENT_EVENTS` can be specified.
we can change it with a variable.

Please let me know if you have a better naming scheme.
@officel
Copy link
Contributor Author

officel commented Dec 8, 2023

Once you have decided on the naming and code direction, decide how to fix ssm_houcekeeper.
Are you okay like that?

@officel
Copy link
Contributor Author

officel commented Dec 11, 2023

@npalm how about this?

Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@officel thx, the variable enable_event_rule_binaries_syncer used on the top-level module and multi-runner. Can you update those as well. See also CI errors

modules/runner-binaries-syncer/variables.tf Show resolved Hide resolved
officel and others added 3 commits December 16, 2023 22:09
I'm sorry I didn't check it out better.

- Written similar to `variables.deprecated.tf`
- top-level `versions.tf` did not know if it should be corrected...
@npalm
Copy link
Member

npalm commented Dec 18, 2023

@officel can you also update this file? modules/multi-runner/runner-binaries.tf line 23, in module "runner_binaries"

@officel
Copy link
Contributor Author

officel commented Dec 19, 2023

@npalm maybe green?

terraform-aws-github-runner $ gg enable_event_rule_binaries_syncer
README.md:524:| <a name="input_enable_event_rule_binaries_syncer"></a> [enable\_event\_rule\_binaries\_syncer](#input\_enable\_event\_rule\_binaries\_syncer) | DEPRECATED: Replaced by `state_event_rule_binaries_syncer`. | `bool` | `null` | no |
variables.deprecated.tf:12:variable "enable_event_rule_binaries_syncer" {
variables.deprecated.tf:17:    condition     = var.enable_event_rule_binaries_syncer == null

@npalm
Copy link
Member

npalm commented Dec 19, 2023

We almost there, you need to run a terraform init -upgrade upgrade on the terraform examples failing in CI. The version is locked on a provider version not supporting the new field.

@officel
Copy link
Contributor Author

officel commented Dec 20, 2023

Do we need a setup-iam-permissions directory?

terraform-aws-github-runner $ gg -A 1 'hashicorp/aws' -- '**/versions.tf'
examples/arm64/versions.tf:4:      source  = "hashicorp/aws"
examples/arm64/versions.tf-5-      version = "~> 5.27.0"
--
examples/base/versions.tf:4:      source  = "hashicorp/aws"
examples/base/versions.tf-5-      version = "~> 5.27.0"
--
examples/default/versions.tf:4:      source  = "hashicorp/aws"
examples/default/versions.tf-5-      version = "~> 5.27.0"
--
examples/ephemeral/versions.tf:4:      source  = "hashicorp/aws"
examples/ephemeral/versions.tf-5-      version = "~> 5.27.0"
--
examples/multi-runner/versions.tf:4:      source  = "hashicorp/aws"
examples/multi-runner/versions.tf-5-      version = "~> 5.27.0"
--
examples/permissions-boundary/setup/versions.tf:4:      source  = "hashicorp/aws"
examples/permissions-boundary/setup/versions.tf-5-      version = "~> 5.27.0"
--
examples/permissions-boundary/versions.tf:4:      source  = "hashicorp/aws"
examples/permissions-boundary/versions.tf-5-      version = "~> 5.27.0"
--
examples/prebuilt/versions.tf:4:      source  = "hashicorp/aws"
examples/prebuilt/versions.tf-5-      version = "~> 5.27.0"
--
examples/ubuntu/versions.tf:4:      source  = "hashicorp/aws"
examples/ubuntu/versions.tf-5-      version = "~> 5.27.0"
--
examples/windows/versions.tf:4:      source  = "hashicorp/aws"
examples/windows/versions.tf-5-      version = "~> 5.27.0"
--
modules/ami-housekeeper/versions.tf:6:      source  = "hashicorp/aws"
modules/ami-housekeeper/versions.tf-7-      version = "~> 5.27.0"
--
modules/download-lambda/versions.tf:6:      source  = "hashicorp/aws"
modules/download-lambda/versions.tf-7-      version = "~> 5.27.0"
--
modules/multi-runner/versions.tf:6:      source  = "hashicorp/aws"
modules/multi-runner/versions.tf-7-      version = "~> 5.27.0"
--
modules/runner-binaries-syncer/versions.tf:6:      source  = "hashicorp/aws"
modules/runner-binaries-syncer/versions.tf-7-      version = "~> 5.27.0"
--
modules/runners/pool/versions.tf:6:      source  = "hashicorp/aws"
modules/runners/pool/versions.tf-7-      version = "~> 5.27.0"
--
modules/runners/versions.tf:6:      source  = "hashicorp/aws"
modules/runners/versions.tf-7-      version = "~> 5.27.0"
--
modules/setup-iam-permissions/versions.tf:6:      source  = "hashicorp/aws"
modules/setup-iam-permissions/versions.tf-7-      version = "~> 5.27.0"
--
modules/ssm/versions.tf:6:      source  = "hashicorp/aws"
modules/ssm/versions.tf-7-      version = "~> 5.27.0"
--
modules/webhook/versions.tf:6:      source  = "hashicorp/aws"
modules/webhook/versions.tf-7-      version = "~> 5.27.0"

@npalm
Copy link
Member

npalm commented Dec 20, 2023

Do we need a setup-iam-permissions directory?

terraform-aws-github-runner $ gg -A 1 'hashicorp/aws' -- '**/versions.tf'
examples/arm64/versions.tf:4:      source  = "hashicorp/aws"
examples/arm64/versions.tf-5-      version = "~> 5.27.0"
--
examples/base/versions.tf:4:      source  = "hashicorp/aws"
examples/base/versions.tf-5-      version = "~> 5.27.0"
--
examples/default/versions.tf:4:      source  = "hashicorp/aws"
examples/default/versions.tf-5-      version = "~> 5.27.0"
--
examples/ephemeral/versions.tf:4:      source  = "hashicorp/aws"
examples/ephemeral/versions.tf-5-      version = "~> 5.27.0"
--
examples/multi-runner/versions.tf:4:      source  = "hashicorp/aws"
examples/multi-runner/versions.tf-5-      version = "~> 5.27.0"
--
examples/permissions-boundary/setup/versions.tf:4:      source  = "hashicorp/aws"
examples/permissions-boundary/setup/versions.tf-5-      version = "~> 5.27.0"
--
examples/permissions-boundary/versions.tf:4:      source  = "hashicorp/aws"
examples/permissions-boundary/versions.tf-5-      version = "~> 5.27.0"
--
examples/prebuilt/versions.tf:4:      source  = "hashicorp/aws"
examples/prebuilt/versions.tf-5-      version = "~> 5.27.0"
--
examples/ubuntu/versions.tf:4:      source  = "hashicorp/aws"
examples/ubuntu/versions.tf-5-      version = "~> 5.27.0"
--
examples/windows/versions.tf:4:      source  = "hashicorp/aws"
examples/windows/versions.tf-5-      version = "~> 5.27.0"
--
modules/ami-housekeeper/versions.tf:6:      source  = "hashicorp/aws"
modules/ami-housekeeper/versions.tf-7-      version = "~> 5.27.0"
--
modules/download-lambda/versions.tf:6:      source  = "hashicorp/aws"
modules/download-lambda/versions.tf-7-      version = "~> 5.27.0"
--
modules/multi-runner/versions.tf:6:      source  = "hashicorp/aws"
modules/multi-runner/versions.tf-7-      version = "~> 5.27.0"
--
modules/runner-binaries-syncer/versions.tf:6:      source  = "hashicorp/aws"
modules/runner-binaries-syncer/versions.tf-7-      version = "~> 5.27.0"
--
modules/runners/pool/versions.tf:6:      source  = "hashicorp/aws"
modules/runners/pool/versions.tf-7-      version = "~> 5.27.0"
--
modules/runners/versions.tf:6:      source  = "hashicorp/aws"
modules/runners/versions.tf-7-      version = "~> 5.27.0"
--
modules/setup-iam-permissions/versions.tf:6:      source  = "hashicorp/aws"
modules/setup-iam-permissions/versions.tf-7-      version = "~> 5.27.0"
--
modules/ssm/versions.tf:6:      source  = "hashicorp/aws"
modules/ssm/versions.tf-7-      version = "~> 5.27.0"
--
modules/webhook/versions.tf:6:      source  = "hashicorp/aws"
modules/webhook/versions.tf-7-      version = "~> 5.27.0"

iam premission module is only an example if you would like to setup permission budnaries

@npalm
Copy link
Member

npalm commented Dec 20, 2023

Can you run this script in the root of the repo will will fix all the lock files

for dir in $(find . -name ".terraform.lock.hcl" -exec dirname {} \;); do
    echo "Upgrading modules in $dir"
    (cd "$dir" && terraform init -upgrade)
done

@officel
Copy link
Contributor Author

officel commented Dec 20, 2023

@npalm

Sorry.
We usually don't maintain .terraform.lock.hcl at our workplace and always run it with init -upgrade, so i forgot it existed... 🙇

@npalm
Copy link
Member

npalm commented Dec 20, 2023

CI is 😄

Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@officel thx for your effort

@npalm npalm changed the title fix: deprecate aws_cloudwatch_event_rule.is_enabled fix: Replace deprecate aws_cloudwatch_event_rule.is_enabled, requires provide upgrade Dec 20, 2023
@npalm npalm merged commit 3c78f65 into philips-labs:main Dec 20, 2023
36 checks passed
@officel
Copy link
Contributor Author

officel commented Dec 20, 2023

@npalm
Thank you very much, too.
I learned a lot. 👍

npalm pushed a commit that referenced this pull request Dec 20, 2023
🤖 I have created a release *beep* *boop*
---

##
[5.6.0](v5.5.2...v5.6.0)
(2023-12-20)

> Release can require upgrade your AWS Terraform provider


### Features

* upgrade lambda runtime from 18x to 20x
([#3682](#3682))
([02dd3e6](02dd3e6))


### Bug Fixes

* **lambda:** bump @aws-lambda-powertools/tracer from 1.16.0 to 1.17.0
in /lambdas
([#3675](#3675))
([b3536f7](b3536f7))
* **lambda:** bump the aws group in /lambdas with 5 updates
([#3671](#3671))
([677965d](677965d))
* **lambda:** bump the octokit group in /lambdas with 1 update
([#3672](#3672))
([67facac](67facac))
* Replace deprecate aws_cloudwatch_event_rule.is_enabled, requires
provide upgrade
([#3655](#3655))
([3c78f65](3c78f65))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: forest-releaser[bot] <80285352+forest-releaser[bot]@users.noreply.github.com>
@@ -4,7 +4,7 @@ terraform {
required_providers {
aws = {
source = "hashicorp/aws"
version = "~> 5.2"
version = "~> 5.27.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

@npalm Was this really required? Didn't you actually mean ~> 5.27?

Copy link
Contributor

Choose a reason for hiding this comment

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

or actually @officel ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jizi maybe yes?

This PR was satisfied where at a minimum the state property indicated a valid version.
You may be right.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but upgrading to the latest github-runner module version actually downgraded the aws module which is probably not what you wanted

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.

3 participants