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.

Sign up for GitHub

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

Jump to bottom

Random suffix to s3 bucket #252

Merged
merged 13 commits into from
Oct 12, 2020
Merged

Random suffix to s3 bucket #252

merged 13 commits into from
Oct 12, 2020

Conversation

fliphess
Copy link
Contributor

@fliphess fliphess commented Sep 1, 2020

Hey Niek!

Feel free to reject this if you don't think it's useful :) I needed it to quickly recreate the module multiple times without having to change the bucket name in every run

(as a result of testing some cloud-init userdata changes in our setup).

Description

This change adds the option to append a random string suffix to the S3 bucket name.
As bucket names can only exist once, it's not possible to destroy the module and reinstall after without config changes as the previously used bucket name is not free for use anymore after teardown. By adding a random string to the bucket name this problem is solved.

This can also be useful if you want to use the same configuration for multiple regions.

Migrations required

NO - Not if cache_bucket_set_suffix is not set to true.

Verification

Not yet! Will test later this week.

Documentation

^ Is this stil actual? I could find the directory anymore. Otherwise I updated the README.md.

@kayman-mk
Copy link
Collaborator

Shouldn't happen too often but makes life easier. +1

npalm
npalm reviewed Sep 2, 2020
View reviewed changes
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Looks good, will run a check in the next days

@npalm npalm self-requested a review September 6, 2020 12:11
npalm
npalm requested changes Sep 6, 2020
View reviewed changes
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Just a few comments

modules/cache/main.tf Outdated Show resolved Hide resolved
modules/cache/main.tf Show resolved Hide resolved
modules/cache/main.tf Outdated Show resolved Hide resolved
@npalm
Copy link
Collaborator

npalm commented Sep 14, 2020

@fliphess did you already had time to have a look on my comments?

@fliphess
Copy link
Contributor Author

@npalm Hey Niek, Sorry, for the late response, I'm on vacation for a few weeks so not working much on terraform at the moment, which is why I didn't do anything with your suggestions so far yet. It is on my todo list at work. (This is my last week, coming Friday it's back to work again)

I'll fixup the code with your suggestions in the beginning of next week if that's okay.

@npalm
Copy link
Collaborator

npalm commented Sep 16, 2020

@fliphess enjoy you holidays

@npalm
Copy link
Collaborator

npalm commented Oct 8, 2020

@fliphess just a kindly reminder. If you have some time would be great if you can fix minor comment

@fliphess
Copy link
Contributor Author

fliphess commented Oct 9, 2020 •
edited
Loading

@npalm Oh thanks for the reminder! It completely slipped my mind! 🤦
I've just pushed the requested changes and rebased on develop. Let me know if this suffices!

@npalm
Copy link
Collaborator

npalm commented Oct 9, 2020

@fliphess thx will check asap

npalm
npalm requested changes Oct 10, 2020
View reviewed changes
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@fliphess variable name in main.tf of root module is not mahcing, created suggestions in the PR.

modules/cache/variables.tf Outdated Show resolved Hide resolved
modules/cache/main.tf Outdated Show resolved Hide resolved
modules/cache/main.tf Outdated Show resolved Hide resolved
npalm
npalm approved these changes Oct 12, 2020
View reviewed changes
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@fliphess thanks!

@npalm npalm merged commit d38b078 into cattle-ops:develop Oct 12, 2020
npalm added a commit that referenced this pull request Jan 13, 2021
@fliphess @npalm
feat: Random suffix to s3 bucket (#252)
825ffeb 
* Set a random suffix on the bucket name for easy recreation

* Add new module option to main module

* Update readme

* Fix oopsy while copy pasting

* Replace a tab with 2 spaces

* Use format() for both string formattings

* Make the linter happy

* Rename variable

* Set a count on random_string so it's only used when applicable

* update readme for cache module

* Update modules/cache/variables.tf

Co-authored-by: Niek Palm <[email protected]>

* Update modules/cache/main.tf

Co-authored-by: Niek Palm <[email protected]>

* Update modules/cache/main.tf

Co-authored-by: Niek Palm <[email protected]>

Co-authored-by: Niek Palm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npalm npalm npalm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fliphess @kayman-mk @npalm