#pr-reviews (2024-06)

Hi @Maurice Just asked our engineers to have a look at this

I am still looking into the VPC issue, but regardless, I think we are not going to approve this PR. It is a breaking change with insufficient justification for it. Null values are already allowed, and no changes are needed for that.

The VPC error was in Feb, and the situation causing it was resolved in the beginning of April. Nothing left to do.

@Gabriela Campana (Cloud Posse) Thanks!

@Jeremy G (Cloud Posse) I can’t retrigger the tests, so I’d need some help for this. Anyways, as already discussed in the PR, I’ll update it with more context about why it is needed - ideally I notice that it isn’t and can close it out.

@Gabriela Campana (Cloud Posse) can you help follow up with this one?

Asked our team to follow-up on the error message
https://github.com/cloudposse/actions/actions/runs/9371790219/job/25801835125#step<i class="em em-9"terratest
Process completed with exit code 2. terratest
Node.js 16 actions are deprecated. Please update the following actions to use Node.js 20: actions/checkout@v3.

OK, I fixed the problem and did another push. It’s waiting for another approval: https://github.com/cloudposse/terraform-aws-rds-cluster/actions/runs/9372514860

Bumped this up internally today

Thanks @Gabriela Campana (Cloud Posse) looks like it was merged!

Thanks for creating this PR! This looks good to me, but we can make use of the optional variable syntax to easily support both types of object with use_latest_restorable_time or with restore_to_time

left a comment on the PR. Feel free to to @ me here!

thanks! pushing up the change in a sec :slightly_smiling_face: wasn’t sure if the lack of using optional params there was legacy or intentional

likely legacy

any thoughts on the default for source_cluster_identifier being 120m? It’s a required parameter in the aws_rds_cluster resource and it looks like it was unintentionally copied and pasted from the timeouts block above

(i.e. should we just make it required and remove the optional / default value?)

oh I see! Yeah that’s definitely a mistake. Looking, one second

how about making source_cluster_identifier not optional with no default?

yeah that’s what I was thinking

it shouldn’t be a breaking change either because there’s no way anyone is using the default value lol

yeah agreed

cool, thanks for the review! the PR is up to date now and ready for another look

np, great! tests are running now. Will approve once / if they pass

thanks! I also noticed that the default value for restore_type (copy-on-write) differs from the default in the aws_rds_cluster resource (full-copy). Is it worth noting that difference in the description?

I think full-copy is required for restoring from a continuous backup

It wouldnt hurt to mention, but it’s not required

gotcha, will leave it as-is then thanks!

thanks again for the review! this was my first open-source contribution is the release process automated?

ah i see it’s already out, nice

no problem and thanks for helping out!

I have to revisit this tomorrow :weary: when instantiating the module, I specify a timestamp for restore_to_time, and it conflicts with use_latest_restorable_time regardless of whether I set it explicitly to false, or null, in which case it defaults to true

I was assuming that Terraform would detect that it’s not a conflict if you specify the default value explicitly (in this case, the default for use_latest_restorable_time is false but it conflicts unless you set it to null)

oh I see. Yeah we’ll probably have to dynamically create each block based on which input is specified

Took another stab at this and tested all of the permutations locally with a project I’m working on https://github.com/cloudposse/terraform-aws-rds-cluster/pull/217

cc @Gabriela Campana (Cloud Posse)

Hi @Quentin BERTRAND Just asked our engineers to have a look at this

@Quentin BERTRAND Thank you for this PR.

As you probably know, PRs

• #178
• #180
• #182 have already started on this, but were missing a lot.

I think you have this most of the way there, although I do see some things that need fixing. You did get it far enough down the field that I think I can take it the rest of the way. Follow up on Thursday if you don’t see progress by then.

Go ahead and take a look at the initial support for EKS on AL2023 and share your constructive comments.

Nice! I just saw last week that replacing the image_id with a public ssm param was the way to keep amis up to date. Very nice to see it happening in the eks module too

@Quentin BERTRAND You said:
Instances created with a launch template and whose AMI is specified in the template do not have kubelet/nodeadm configuration, so do not join the cluster Could you be more specific? Which AMI, which AMI Type, which K8s version, which region? This may not be something the module can do anything about.

The module creates a launch template with an AMI ID unless you supply a launch template yourself. The Terratest (and other testing) confirm that these nodes join the cluster, with EKS supplying the configuration if you do not include any userdata. If you do include userdata for AL2023, then you have to include all the required elements of the NodeConfig as part of it.

Other possibilities include Security Group issues or using the wrong service CIDR (it’s confusingly vague).

Hello, My cluster is running 1.30 K8S version in eu-west-3 region.

Here are some of my values:

ami_specifier         = "amazon-eks-node-al2023-x86_64-standard-1.30-v20240605"
ami_type              = "AL2023_x86_64_STANDARD"

associate_cluster_security_group = false

associated_security_group_ids = [
  dependency.sg_default.outputs.id,
  dependency.sg_eks_nodes.outputs.id
]

node_group_terraform_timeouts = [{
  create = "1h"
  update = "1h"
  delete = "1h"
}]

block_device_mappings = [
  {
    "delete_on_termination" : true,
    "device_name" : "/dev/xvda",
    "encrypted" : true,
    "volume_size" : 20,
    "volume_type" : "gp3"
  }
]

tags = {
  "k8s.io/cluster-autoscaler/node-template/label/nodetype" = "default"
}

I have no problem with 2.12.0 module version.

I tried to create a new node group, with min size and desired capacity set to 0 , the creation of node group end in failure.

NodeCreationFailure	Instances failed to join the kubernetes cluster

ami_type = "AL2023_x86_64_STANDARD"

But in EKS console, AMI type is CUSTOM

@Jeremy G (Cloud Posse) Thanks for you GitHub comment ;
If you supply an AMI ID in the launch template, then EKS does not add anything to the userdata, since it cannot know what the AMI is expecting. If you do not supply an AMI ID in the launch template, EKS creates a copy of the launch template, adding the AMI ID and its userdata to set up the node. Maybe the module could let us choose between set ami_specifier or ami_version?

I’m am going to revert the change from ami_release_version to ami_specifier, because ami_release_version is an input to aws_node_group. (Even though I wrote most of this code, it was 4 years ago and I forgot a lot.)

@Quentin BERTRAND The AL2023 support PR is ready to be reviewed again.

@Jeremy G (Cloud Posse) Thank you for this!

Tested on a staging env with AL2023, it works as expected

cloudposse/eks-node-group/aws v3.0.0 released with AL2023 support and several other minor improvements and bug fixes.

Hi @Yangci Ou Just asked our team to review this PR

Hi @Frank Just asked our team to review this PR

@Hans D, @RB the GitHub automation tagged you as reviewers for this PR.

The code looks good to me.

The tests are failing due to what looks like a permission limitation on the cloudflare api token. I added a comment and cc’ed other cloudposse members who have more permission to cloudflare

Thanks.

@Andriy Knysh (Cloud Posse) do you suppose this is something you can fix with the cloudflare token, or is there anything else I might do to make this test pass?

let us check this, we’ll let you know

@Ben Smith (Cloud Posse)

Hey folks, any update regarding these PRs? Do you want me to make any change on them, or in their flows?

@Gabriela Campana (Cloud Posse)

Hey @Diego Rabatone Oliveira sorry for the hold up, taking a look at these now

@Ben Smith (Cloud Posse) it seems https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/44 failed test/terratest/opentofu

That link gave a 404, which PR are you referring to?

[Notification Policy] Imrprove module

@Diego Rabatone Oliveira

• [escalation] Single recipient per rule #58 has tests failing

Commit pushed to Escalation PR. I’ll take a look at the other one.

Regarding the Notification Policy PR, I don’t get what is wrong over there. It seems it failed in an unrelated tests. I wonder if that was just a opportunistic failure or something like that.

Yeah the notification PR i’m working on a fix for - theres an outdated test. will update test suite then re-review and update PRs

opentofu and TF applies are clashing

Oh! Make sense….

PRs updated (with some lint fixes):

• https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/58

• https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/61

• https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/60

• https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/62

@Ben Smith (Cloud Posse)

I know you eventually still need to handle the CI issue btw

Just merged in a PR that will run all tests, only ones that we cannot test are Escalations, config, and some others. but should be a bit more comprehensive

PRs updated

Regarding https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/58#pullrequestreview-2194658191 - I was thinking of doing this in the test PR but decided it was out of scope to fix. Right now we have some inconsistencies - modulesconfig/escalations uses rules but modules/escalation uses rule with out a dynamic which it looks like you are fixing, we’ll want to update rule to `rules across the board if this PR is willing to take that on

Hey Ben, sure thing! It does make sense and probably I missed this change while I was splitting the changes into multiple PRs. I’ll review it to enforce rules both on config and on the module itself.

Btw, over there I have a doubt, and it is a hcl doubt to be honest.

Within the dynamic "rules" block we have another block recipient , and the type attribute over there. To fill it, should I use rules.value.recipient.type or rules.value.recipient["type"]? (locally a saw an issue with it)

should be rules.value.recipient.type. We might also want to setup enforced types but perhaps thats another day.

did you see issues with rules.value.recipient.type?

what is the for_each line for the dynamic rules

for_each = try([each.value.rules], [])

that should probably be for_each = try(each.value.rules, []) since rules is now an array type (I changed that yesterday because we didn’t have it as an array but an object in config

I’m wondering if the mistake is in the yaml definition: and if it should be:

recipient:
- type: ...
  name: ...

instead of

recipient:
  type: ...
  name: ...

But, indeed, you are right, the for_each is wrong.

yeah

  rules:
  - condition: if-not-acked
    notify_type: default
    delay: 0
    recipient:
     type: team
     team_name: acme.dev.some-service

Rules is an array (we have many) and is a dynamic, recipient is a singular object which we found from trying to hit multiple and getting an opsgenie error

Reviewed most of the PRs. Couple questions comments or updates for each

OMG! I didn’t know about the opsgenie-team module beforehand

I spent alot of time building that component. in general look to terraform-aws-components for pre-built stuff (it’s opinionated though) and modules are our building blocks

It uses this module VERY heavily.

I can imagine!

Well, I’ll answer there anyway, thanks for the reviews, feedback and attention As soon as I have the PRs updated I’ll let you know

I’ll try to get to them - I’m on PTO next week though so i’ll be looking throughout this week and in august

No problem at all! Just make a good usage of your PTO If I can’t finish until then.

I did my best over there - although I have to be honest, I’m totally disappointed with myself for not finding the opsgenie-team before

(taking the opportunity…

I saw on opsgenie-team an example of “business hours” definition as:

# 9-5 Mon-Fri
business_hours: &business_hours
  type: "weekday-and-time-of-day"
  restrictions:
    - start_hour: 9
      start_min: 00
      start_day: "monday"
      end_hour: 17
      end_min: 00
      end_day: "friday"

This is technically wrong, right? this is from Monday 9h until Friday 17h instead of From Monday to Friday, from 9h to 17h.

@Ben Smith (Cloud Posse)

I’m totally disappointed with myself for not finding the opsgenie-team before Maybe we could do a better job advertising it —

You might be right, I assumed it was 9h -> 17h on each day from monday -> friday, but I could see how it could be as you said. I haven’t checked deploying that in a while if that’s how it deploys. if it does deploy incorrectly then we should update the example to be

# 9-5 Mon-Fri
business_hours: &business_hours
  type: "weekday-and-time-of-day"
  restrictions:
    - start_hour: 9
      start_min: 00
      start_day: "monday"
      end_hour: 17
      end_min: 00
      end_day: "monday"
    - start_hour: 9
      start_min: 00
      start_day: "tuesday"
      end_hour: 17
      end_min: 00
      end_day: "tuesday"
...

I avoided doing that being hopeful the other way is how it worked

Hi @Eligio Mariño https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/100 is approved

Thank you @Gabriela Campana (Cloud Posse) for the update. It was mentioned that we’ll wait a few more days for a few changes from other contributor. I’m looking forward to the merge of this intermediate PR, so we can focus in my original PR https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/94

Sure. Go ahead. Thank you so much for taking it into consideration.

I actually got it updated already. @Eligio Mariño Please take a look and let me know if you have any ideas for improving it.

I took away some of your utility functions as I didn’t see them likely to be used. Instead I have a simple testRunner which supplies defaults and a more complex explicitTestRunner which allows (and requires) more configuration.

Thank you @Jeremy G (Cloud Posse)! Your changes look great. I misunderstood the instructions, I saw the testRunner and I thought the intention was to split it into smaller reusable functions so the test inputs, outputs, and cleanup were readable in only one function. My approach was to remove those two functions per test and leave only one, like TestExamplesComplete and testExamplesComplete , which are necessary once testRunner is introduced. I understand your reasoning, and I think they make sense because you are already using testRunner in other modules.

Your changes look good to me. I also saw that the PR is approved, but it was mentioned that we’ll wait for other changes. I’m really looking forward to all the improvements and merging this intermediate PR, to focus again on my original PR.

@Eligio Mariño PR 100 has been merged.

Amazing! Thank you @Jeremy G (Cloud Posse).

@Gabriela Campana (Cloud Posse) could you please take a look?

@Erik Osterman (Cloud Posse) I can see you already escalated this via https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/258#issuecomment-2100641731

worth trying again?

@Gabriela Campana (Cloud Posse)

Just asked our engineers to have a look at this

Hi @Moritz @Igor Rodionov re-ran terratest on Thursday, and it seems terrates/opentofu and terratest/terraform are failing.

what is the way forward in that case?

So the automatic update by Dependabot shows that upgrading to the v5 provider is a breaking change.

It will require someone to open a PR to address the underlying changes. Cloud Posse predominantly performs services on behalf of customers, so if this is something you need, we recommend opening a PR to address the breaking changes.

(we don’t have a customer sponsoring this work for the ECS app)

I can take a look, it might also be an issue with the “complete” example used for the test.

Could someone please run terratest for some of module dependency PRs (and merge if it passes), the bot ignored the trigger comment by mergify? https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/279 https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/272

this might also be an intermittent issue/race condition present in older provider versions: https://github.com/hashicorp/terraform-provider-aws/issues/427 https://github.com/hashicorp/terraform/issues/10737

It’s even documented here: https://github.com/cloudposse/terraform-aws-components/blob/main/deprecated/aws/ecs/README.md#objectnotfoundexception-n[…]ered-for-service-namespace

Could someone please re-run terratest for the v5 provider PR?

I kicked those tests off

it’s looking good now

@mihai.plesa @Moritz I fixed that

https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/258

Will merge now

great work, thank you again

@Andriy Knysh (Cloud Posse)

@Quentin BERTRAND thanks, approved and merged