#pr-reviews (2024-11)

@Igor Rodionov

Cc @Gabriela Campana (Cloud Posse)

@Igor Rodionov please lmk when you address this

@Erik Osterman (Cloud Posse) May I have an approval https://github.com/cloudposse/build-harness/pull/390 ?

@Yangci Ou fixed and released https://github.com/cloudposse/terraform-aws-alb/releases/tag/v2.1.0

awesome, thanks so much! appreciate the quick help!!

If you’re affected by this in any other repos, let us know and we can retrigger the builds

Please tag me, if it happens once again, @Yangci Ou

Thanks!

Thank you!

Thank you!

Merge conflicts that must be resolved first

Thanks Veronika. I fixed the conflicts.

Before merging, i was hoping to chat with someone about this comment to see what the proper step is for this.

https://github.com/cloudposse/terraform-aws-s3-bucket/pull/258#pullrequestreview-2434928344

cc @Jeremy G (Cloud Posse) if you have a sec

@RB I would prefer to reject (or at least place on indefinite hold) this PR, because it is a breaking change that adds no new functionality

We can leave it open with do not merge and incorporate it into the next major version update, but I don’t think it warrants a version update and breaking change on its own. I want to reserve those for new features that are incompatible with old features or removing deprecated features.

Yes i figured this but was wondering if we can do something like put the old var in variables-deprecated and if either is used, make it have the same impact, that way it would conform to cloudposse naming conventions without a breaking change, no?

It’s on my to-do list to document Cloud Posse’s policy on exactly that. @Erik Osterman (Cloud Posse) How do you want to handle this?

I think we need to use release branches and merge this into those future release branches

won’t adding the major label have the same effect

@Erik Osterman (Cloud Posse) I meant, I would prefer to defer this change until we had more significant breaking changes, because I don’t have a problem with the current variable name. If you think it should go out now with a major version bump, then I will update the docs sooner rather than later about how to deprecate the old variable.

Couldn’t it go out without a major bump if the new var is added and the old vat is moved to variables-deprecated ?

Then later when the major release is ready, the variables-deprecated can be wiped ?

Thank you!

we will merge this PR next week https://github.com/cloudposse-terraform-components/aws-spacelift-admin-stack/pull/4#issuecomment-2486671429

np, got this info from Igor already thanks!

@Igor Rodionov

Thanks Gabriela, is there a possible timeline for when this can be reviewed and merged? thanks @Gabriela Campana (Cloud Posse) @Igor Rodionov

Hi @hamza-25 We can not promise on an ETA, as client work is prioritized over community support. I’ve tagged our engineers on the PRs. This is a best effort by a small team. We appreciate your persistence, and we’ll get to it when we can.

@hamza-25 thanks for your contribution. the PR merged and released https://github.com/cloudposse/terraform-aws-ec2-instance/releases/tag/v1.6.1

Thanks a lot, both of you!

I can now get rid of my own local changes much appreciated

Thanks!

@Igor Rodionov

@z0rc3r these were absolutely not garbage; these were great recommendations. We should strive for better variable descriptions, so the user has everything they need to use the variable.

As merged, the descriptions are incomplete.

We are using these suggestions ourselves, and improving the code quality of #amtos https://github.com/cloudposse/atmos/pulls?q=is%3Apr+is%3Aclosed+is%3Amerged

Plus, CodeRabbit learns from feedback. If you have constructive criticism of why they are not good, you can share that.

Case in point:

-  description = "Configuration block for the support policy to use for the cluster"
+  description = "Configuration block for the support policy to use for the cluster. The support_type can be set to 'Standard' or 'Extended' (default). For more information, see <https://docs.aws.amazon.com/eks/latest/userguide/service-quotas.html#service-quotas-eks-support>"

I prefer the second description over the first one.

one man’s trash is another man’s treasure

I’m not dealing with llm recomndations

Feel free to close the pr

@z0rc3r ups. I merged it couple hours ago

https://github.com/cloudposse/terraform-aws-eks-cluster/releases/tag/v4.5.0

Honestly, what’s bad about the recommendations? What’s trash about them?

Links are incorrect, recommended values use wrong capitalization. In the end they rehash the official aws docs in a slightly convoluted way.

Fair point about invalid links, if true. However, showing what the acceptable values are is

Hard disagree. My preference is to leave this on provider level, when module replicates behaviour from the provider. Which is the case here.

Thanks for sharing your views.

As an end user of other modules I often don’t care what is in variables description. I always read the code to understand it’s behaviour and how resources interact with each other.

General problem of any ai stuff that it can hallucinate things. So in the end it wastes the contributor’s time to do the fact checking. Plus it doesn’t add value. If you want to give more info just link to provider or aws docs.

Often terraform modules have such poor descriptions, reviewing the provider docs is required. But IMO a well-defined module has thoroughly documented variables, outputs, examples, terratests, and README. We often spend longer on everything else than the modules themselves.

@johncblandii

Could it be merged please? Test passed, @RB also approved it already

@johncblandii bumping this up

@Andriy Knysh (Cloud Posse)