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

upgrade to latest module 'cloudposse/label/null' + fixes #10 #14

Merged
merged 3 commits into from
Jan 26, 2023

Conversation

gberenice
Copy link
Member

@gberenice gberenice commented Jan 23, 2023

Info

@gberenice gberenice requested a review from Gowiem January 23, 2023 13:53
main.tf Outdated
enabled = var.session_logging_enabled && var.session_logging_encryption_enabled && var.session_logging_kms_key_arn == ""
context = module.logs_label.context
enabled = var.session_logging_enabled && var.session_logging_encryption_enabled && length(var.session_logging_kms_key_arn) == 0
context = module.this.context
Copy link
Member

Choose a reason for hiding this comment

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

Because we're keeping the same context EXCEPT the attributes, we'd break existing users usage of this module when they upgrade. Let's add that attributes, so we don't mess up folks usage of this module.

Suggested change
context = module.this.context
context = module.this.context
attributes = attributes = compact(concat(["logs"], module.this.attributes))

Copy link
Member Author

Choose a reason for hiding this comment

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

@Gowiem I decided to keep label modules for logs, role and ASG in order to avoid undesired recreation of the resources caused by renaming. We could circle back on the refactoring like this in case we work on other major updates sometime later that will require the replacement.

main.tf Outdated
@@ -320,17 +305,18 @@ resource "aws_launch_template" "default" {
}

resource "aws_autoscaling_group" "default" {
name_prefix = "${module.label.id}-asg"
tags = module.label.tags_as_list_of_maps
name_prefix = module.this.id
Copy link
Member

Choose a reason for hiding this comment

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

Should we use asg_label here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated 👍

@gberenice
Copy link
Member Author

@Gowiem the variable max_instance_lifetime was removed as well, since we're not going to use it as we thought.

With all the above, the plan for existing infra should be like:

  # module.ssm_agent.aws_autoscaling_group.default will be updated in-place
  ~ resource "aws_autoscaling_group" "default" {
      + force_delete_warm_pool    = false
        id                        = "sts-prod-ssm-agent-asg20201203003845865900000004"
        name                      = "sts-prod-ssm-agent-asg20201203003845865900000004"
      ~ tags                      = [
          + {
              + "key"                 = "Attributes"
              + "propagate_at_launch" = "true"
              + "value"               = "asg"
            },
          - {
              - "key"                 = "Name"
              - "propagate_at_launch" = "true"
              - "value"               = "sts-prod-ssm-agent"
            },
          + {
              + "key"                 = "Name"
              + "propagate_at_launch" = "true"
              + "value"               = "sts-prod-ssm-agent-asg"
            },
          + {},
            # (2 unchanged elements hidden)
        ]
        # (23 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Gowiem
Gowiem previously approved these changes Jan 24, 2023
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@gberenice looks great, let's :shipit:!

@gberenice
Copy link
Member Author

@Gowiem I've updated the PR to address #10. Please review.

@gberenice gberenice merged commit 84f133e into master Jan 26, 2023
@gberenice gberenice changed the title upgrade to latest module 'cloudposse/label/null' + adds new variable upgrade to latest module 'cloudposse/label/null' + fixes #10 Jan 26, 2023
@gberenice gberenice mentioned this pull request Jan 26, 2023
@gberenice gberenice deleted the general_improvements branch February 5, 2024 14:57
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.

None yet

2 participants