#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!