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: Added support for tfupdate to update version constraints in Terraform configurations #342

Merged
merged 7 commits into from Apr 13, 2022

Conversation

jrottenberg
Copy link
Contributor

@jrottenberg jrottenberg commented Feb 13, 2022

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

Fixes #328

How can we test changes

Working on an existing module, for example : https://github.com/cloudposse/terraform-aws-sso.git

Prepare the pre-commit config :

repos: # pre-commit autoupdate
  - repo: https://github.com/jrottenberg/pre-commit-terraform
    rev: tfupdate
    hooks:
      - id: tfupdate
        name: tfupdate terraform
      - id: tfupdate
        name: tfupdate provider aws
        args:
          - provider
          - aws   
❯ pre-commit run -a
[...]
tfupdate terraform.......................................................Failed
- hook id: tfupdate
- files were modified by this hook
tfupdate provider aws....................................................Failed
- hook id: tfupdate
- files were modified by this hook

And for the diff :

diff --git a/examples/complete/versions.tf b/examples/complete/versions.tf
index 42f8195..764880f 100644
--- a/examples/complete/versions.tf
+++ b/examples/complete/versions.tf
@@ -1,5 +1,5 @@
 terraform {
-  required_version = ">= 0.13.0"
+  required_version = "1.1.5"
 
   required_providers {
     local = "~> 1.2"
diff --git a/modules/account-assignments/versions.tf b/modules/account-assignments/versions.tf
index c9a3bb8..f470932 100644
--- a/modules/account-assignments/versions.tf
+++ b/modules/account-assignments/versions.tf
@@ -1,10 +1,10 @@
 terraform {
-  required_version = ">= 0.13.0"
+  required_version = "1.1.5"
 
   required_providers {
     aws = {
       source  = "hashicorp/aws"
-      version = ">= 3.26.0"
+      version = "4.0.0"
     }
   }

teraform version and the provider are pinned.

On subsequent run :

❯ pre-commit run -a
[...]
tfupdate terraform.......................................................Passed
tfupdate provider aws....................................................Passed

@jrottenberg jrottenberg changed the title Tfupdate feat: Tfupdate integration Feb 13, 2022
@jrottenberg
Copy link
Contributor Author

Beside the new feature I had to path the __init__.py file since the warning for terraform_docs_replace was leaking on any python hook.

tfupdate uses subcommands no options it was easier to write the hook in python.

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
hooks/tfupdate.py Outdated Show resolved Hide resolved
@jrottenberg jrottenberg force-pushed the tfupdate branch 5 times, most recently from 2c6c813 to 2dd8fcb Compare February 20, 2022 18:13
@jrottenberg
Copy link
Contributor Author

@yermulnik @MaxymVlasov , ok updated the PR as recommended, let me know if it needs anything else.

Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

Let's use common functions to parse args and then use function per_dir_hook_unique_part and function run_hook_on_whole_repo as replace for function tfupdate_

First should work only for dirs with changes, second - recursive for whole repo

Check https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/terrascan.sh as an example

@jrottenberg
Copy link
Contributor Author

jrottenberg commented Feb 21, 2022

Let's use common functions to parse args and then use function per_dir_hook_unique_part and function run_hook_on_whole_repo as replace for function tfupdate_

First should work only for dirs with changes, second - recursive for whole repo

Check https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/terrascan.sh as an example

tfupdate behaves differently than terrascan, it really runs only against the module as a whole, not individual files. It makes sure the version of terraform or the various providers is pinned to the latest when tfupdate ran.

The version lookup is a bit expansive and I'd go against running it multiple times, as it will give the same value, but could be considered as abuse from github (it uses the api to extract the latest published version).

I'll make the change to use common::parse_cmdline

@jrottenberg
Copy link
Contributor Author

Ok it is consistent with other hooks in term of passing options but seems a bit verbose args of args, when the command itself (tfupdate) takes only a subcommands and parameters. Either way, let me know if you prefer this version, I'll squash and push.

Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

  1. Please add installation instructions to README (for all OSes and param in Docker) and Dockerfile.

tfupdate behaves differently than terrascan, it really runs only against the module as a whole, not individual files. It makes sure the version of terraform or the various providers is pinned to the latest when tfupdate ran.

The version lookup is a bit expansive and I'd go against running it multiple times, as it will give the same value, but could be considered as abuse from github (it uses the api to extract the latest published version).

It runs on per-dir basic, that same as for terrascan and many other hooks.

# consume modified files passed from pre-commit so that
# hook runs against only those relevant directories
local index=0
for file_with_path in "${files[@]}"; do
file_with_path="${file_with_path// /__REPLACED__SPACE__}"
dir_paths[index]=$(dirname "$file_with_path")
((index += 1))
done

# run hook for each path
for dir_path in $(echo "${dir_paths[*]}" | tr ' ' '\n' | sort -u); do
dir_path="${dir_path//__REPLACED__SPACE__/ }"
pushd "$dir_path" > /dev/null || continue
per_dir_hook_unique_part "$args" "$dir_path"

So, please use function per_dir_hook_unique_part and function run_hook_on_whole_repo

Just do not use --recurcive in function per_dir_hook_unique_part :)

@jrottenberg jrottenberg force-pushed the tfupdate branch 2 times, most recently from 3cdd676 to a2d7be3 Compare February 28, 2022 05:07
@jrottenberg
Copy link
Contributor Author

ok @MaxymVlasov I'm going with the flow, let me know if I'm missing anything else.

Thanks for the feedback.

@jrottenberg jrottenberg force-pushed the tfupdate branch 3 times, most recently from 1d86be0 to 32a400d Compare February 28, 2022 05:25
Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

LGTM. Just need to check that all works fine. I'll do that little bit later

@jrottenberg
Copy link
Contributor Author

Argh, I'll fix the pre-commit.

Dockerfile Outdated
if [ "$TFUPDATE_VERSION" != "false" ]; then \
( \
TFUPDATE_RELEASES="https://api.github.com/repos/minamijoyo/tfupdate/releases" && \
[ "$TFUPDATE_VERSION" = "latest" ] && curl -L "$(curl -s ${TFUPDATE_RELEASES}/latest | grep -o -E "https://.+?/tfupdate_.+_linux_amd64.tar.gz")" > tfupdate.tgz \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might be doing something different, but this doesn't work for me on Ubuntu as it returns more than just browser_download_url.
What works for me is e.g. grep -o -E "https://.+?_.+_linux_amd64.tar.gz"

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
hooks/tfupdate.sh Outdated Show resolved Hide resolved
hooks/tfupdate.sh Outdated Show resolved Hide resolved
@jrottenberg jrottenberg force-pushed the tfupdate branch 2 times, most recently from 1908e84 to b7693e8 Compare March 6, 2022 18:29
@jrottenberg
Copy link
Contributor Author

Thank you very much @yermulnik , I was missing the targe on the "tar x".

Let me know what you think @MaxymVlasov

@jrottenberg
Copy link
Contributor Author

@MaxymVlasov very strange, the pre-commit are passing locally, hadolint included.

@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 13, 2022
@jrottenberg jrottenberg force-pushed the tfupdate branch 2 times, most recently from e702952 to 6e1635f Compare April 13, 2022 04:08
@jrottenberg
Copy link
Contributor Author

rebased and ready for review

@MaxymVlasov MaxymVlasov removed the stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 13, 2022
Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

Revert some changes

CHANGELOG.md Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

Great job!
I fixed all things where I had some questions, so now LGTM

Let's wait to merge #362, rebase here and if all pipelines will be happy and @yermulnik will approve PR, we can go to merge it

Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated Show resolved Hide resolved
@antonbabenko antonbabenko changed the title feat: Tfupdate integration feat: Added support for tfupdate to update version constraints in Terraform configurations Apr 13, 2022
@antonbabenko antonbabenko merged commit ef7a0f2 into antonbabenko:master Apr 13, 2022
antonbabenko pushed a commit that referenced this pull request Apr 13, 2022
# [1.66.0](v1.65.1...v1.66.0) (2022-04-13)

### Features

* Added support for `tfupdate` to update version constraints in Terraform configurations ([#342](#342)) ([ef7a0f2](ef7a0f2))
@antonbabenko
Copy link
Owner

This PR is included in version 1.66.0 🎉

@jrottenberg
Copy link
Contributor Author

Thank you !

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.

Support for tfupdate
4 participants