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

feat: location inverse control in bucket name prefix #186

Merged

Conversation

Tensho
Copy link
Contributor

@Tensho Tensho commented Sep 15, 2022

Right now the bucket location always appears in the name. It would be nice to have full control over prefix via respective input variable. Moreover, I'd prefer to have the same for suffix too (there is already example described in example folder). But let's have incremental changes and dedicate this one to location in prefix exclusively.

This change introduces a contract change, ergo it's a breaking change. Major version release is required according to the semver.

Before

module "abc" {
  ...
  location = var.location
  prefix   = var.project_id
  ...
}

You get the location in the bucket name.

After

module "abc" {
  ...
  location = var.location
  prefix     = "${var.project_id}-${var.location}" # Inverse control of prefix value
  ...
}

@Tensho Tensho changed the title Conditional location feat: location inverse control in bucket name prefix Sep 15, 2022
@Tensho
Copy link
Contributor Author

Tensho commented Nov 3, 2022

Could we have this merged please?

@github-actions
Copy link

github-actions bot commented Jan 3, 2023

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Stale label Jan 3, 2023
@Tensho Tensho requested a review from a team as a code owner January 3, 2023 23:22
@Tensho
Copy link
Contributor Author

Tensho commented Jan 3, 2023

@bharathkkb Hey, please could you check this out? PR has was marked as stale and I start to worry this change won't be addressed.

@github-actions github-actions bot removed the Stale label Jan 4, 2023
@g-awmalik
Copy link
Contributor

Thanks for the PR @Tensho. Since this is a breaking change, can you also add an doc here detailing to upgrade to this version? Otherwise LGTM.

Copy link
Contributor

@g-awmalik g-awmalik left a comment

Choose a reason for hiding this comment

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

Please add an upgrade doc for this breaking change.

@Tensho Tensho force-pushed the conditional-location branch 2 times, most recently from f2c9cb9 to f719236 Compare January 6, 2023 16:01
@Tensho
Copy link
Contributor Author

Tensho commented Jan 6, 2023

@g-awmalik Added upgrade doc and rebase everything on the latest upstream master. Let me know if there's anything else you can think of.

@Tensho Tensho force-pushed the conditional-location branch from f719236 to be55023 Compare January 6, 2023 16:05
@g-awmalik
Copy link
Contributor

@g-awmalik Added upgrade doc and rebase everything on the latest upstream master. Let me know if there's anything else you can think of.

Sorry I should've been more clear. This change will go in v4.0 so we'll need a new doc upgrading_to_v4.0.md. Secondly, please explain with code snippets what a v3.x user would have to do to make sure they don't see any changes in their tf plan when they run this module with v4.0. Hopefully that gives you better direction.

@Tensho Tensho force-pushed the conditional-location branch from be55023 to ce94205 Compare January 9, 2023 21:19
@Tensho
Copy link
Contributor Author

Tensho commented Jan 9, 2023

@g-awmalik Done.

@Tensho Tensho force-pushed the conditional-location branch from ce94205 to 479c51b Compare January 9, 2023 21:22
@Tensho Tensho requested a review from g-awmalik January 10, 2023 09:47
@comment-bot-dev
Copy link

@Tensho
Thanks for the PR! 🚀
✅ Lint checks have passed.

Copy link
Contributor

@g-awmalik g-awmalik left a comment

Choose a reason for hiding this comment

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

LGTM

@g-awmalik g-awmalik merged commit dbd3c35 into terraform-google-modules:master Jan 18, 2023
@Tensho Tensho deleted the conditional-location branch March 21, 2023 21:31
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