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: updating AWS Provider and Terraform version #145

Closed
wants to merge 12 commits into from

Conversation

esacteksab
Copy link
Contributor

Changes proposed in this pull request:

  • ci: TFDocs official GHA
  • chore: removing leftovers from terratest
  • ci: updating pre-commit
  • feat: upgrading AWS and Terraform versions

This is similar to this PR.

@rpdelaney
Copy link
Contributor

Why remove the examples?

Also: Merge conflict! Yay!

@esacteksab
Copy link
Contributor Author

Why remove the examples?

Also: Merge conflict! Yay!

Because the examples are remnants that weren't removed when we got rid of terratest. They're not necessary.

@jsclarridge
Copy link
Contributor

jsclarridge commented May 25, 2023

Regarding the version constraints, Terraform's recommended best practice (https://developer.hashicorp.com/terraform/language/expressions/version-constraints#best-practices) is for reusable modules to only specify a minimum version constraint:

Reusable modules should constrain only their minimum allowed versions of Terraform and providers, such as >= 0.12.0. This helps avoid known incompatibilities, while allowing the user of the module flexibility to upgrade to newer versions of Terraform without altering the module.

We previously specified maximum versions in several of our modules, but then I think we switched to only specifying minimum versions.

Either way, this is a good candidate for a broader Architectural Decision Record (ADR) on the topic if we don't already have one.

@rpdelaney
Copy link
Contributor

@esacteksab

Because the examples are remnants that weren't removed when we got rid of terratest. They're not necessary.

They're used by humans, not just terratest, though :(

Copy link
Contributor

@rpdelaney rpdelaney left a comment

Choose a reason for hiding this comment

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

Are the examples out of date? Unhelpful? Worse than nothing? 👀

@rpdelaney
Copy link
Contributor

@esacteksab I might be wrong, but from looking at these examples/ that were removed, I don't see an obvious connection to terratest :(

@esacteksab
Copy link
Contributor Author

Sorry, I've neglected this. I'll be back hopefully by EOD tomorrow (June 23, 2023). Thank you and sorry!

Copy link
Contributor

@rpdelaney rpdelaney left a comment

Choose a reason for hiding this comment

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

YAY :D

Now we get to battle with terraform-docs short circuiting required checks!

@esacteksab
Copy link
Contributor Author

YAY :D

Now we get to battle with terraform-docs short circuiting required checks!

Yeah, I have to be honest. That experience sucked. I'd run pre-commit locally, go to push, couldn't because merge conflicts, pull, make changes, attempt to push, couldn't. It was terrible and if this is what it means to have CI do writes on doc updates, then I'm a hard no and I change my mind. I'll never contribute again because it's too terrible, like worse than why I championed to let the machines do stuff for us. Sorry! I was wrong. I'm team get rid of it. /rant

Copy link
Contributor

@jsclarridge jsclarridge left a comment

Choose a reason for hiding this comment

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

Given some of the recent commits, I don't think the current changes to the following files are needed:

.terraform.lock.hcl
README.md

@esacteksab esacteksab closed this Jul 4, 2023
@esacteksab esacteksab deleted the barry-update-things branch July 4, 2023 12:12
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.

None yet

3 participants