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

fix: Terraform validate - only run tf init if providers directory is missing in .terraform #509

Closed
wants to merge 1 commit into from

Conversation

smelchior
Copy link
Contributor

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

If a terraform states does not include modules, a .terraform/modules directory is not present. For these cases, the tf init is run every time the validate hook runs, which takes extra time + causes issues if e.g. a remote state is used which causes the init to fail.

How can we test changes

I checked it with my repositories as described above, and the tf init did now only run if the .terraform/providers directly is not present.

@smelchior smelchior changed the title fix: only run tf init if providers directory is missing in .terraform fix: terraform validate: only run tf init if providers directory is missing in .terraform Apr 24, 2023
@smelchior smelchior changed the title fix: terraform validate: only run tf init if providers directory is missing in .terraform Fix: terraform validate: only run tf init if providers directory is missing in .terraform Apr 24, 2023
@smelchior smelchior changed the title Fix: terraform validate: only run tf init if providers directory is missing in .terraform fix: Terraform validate - only run tf init if providers directory is missing in .terraform Apr 24, 2023
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.

Makes sense.

@baolsen JFYI (since this was a part of #441)

@yermulnik
Copy link
Collaborator

@MaxymVlasov Can you think of any reason to check both dirs instead of only .terraform/providers/?

@MaxymVlasov
Copy link
Collaborator

If a terraform states does not include modules

If a terraform state does not include providers, what then?

@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Apr 24, 2023

I agree that a situation without modules is more common than modules without providers, but let's brainstorm first are there any modules that don't need providers

@yermulnik
Copy link
Collaborator

but let's brainstorm first are there any modules that don't need providers

Output-only modules.

@yermulnik
Copy link
Collaborator

yermulnik commented Apr 24, 2023

Maybe add something like --force-no-init or --ignore-init arg for common::terraform_init() function so that it always returns success when this flag is passed by user? 🤔

@MaxymVlasov
Copy link
Collaborator

Maybe add something like --force-no-init or --ignore-init arg for common::terraform_init() function so that it always returns success when this flag is passed

Hmm, or ask the user to provide a comma-separated list of what paths should be checked, ie:

- --run-tf-init-if-not-exits-any-of=.terraform/providers,.terraform/foobar

@yermulnik
Copy link
Collaborator

Hmm, or ask the user to provide a comma-separated list of what paths should be checked, ie:

Might be a bit of overkilling as of quite rare use case though. Those who would want to provide a list of options, most probably were much more smarter and already use mkdir to circumvent this hook's unwanted behavior 😃

@smelchior
Copy link
Contributor Author

Maybe we can take one step back:
Currently the check if the init is required to run happens before the validate call.
Could the order be switched? So run the validate first, if that fails do the whole process as it is right now?

Creating the "dummy" directory ".modules" was my short term fix, but that is a bit of a pain :-D

@MaxymVlasov
Copy link
Collaborator

@smelchior good idea, let's do it

Here are my errors when no providers and modules provided

➜ time t validate -json
{
  "format_version": "1.0",
  "valid": false,
  "error_count": 4,
  "warning_count": 0,
  "diagnostics": [
    {
      "severity": "error",
      "summary": "Module not installed",
      "detail": "This module is not yet installed. Run \"terraform init\" to install all modules required by this configuration.",
      "range": {
        "filename": "main.tf",
        "start": {
          "line": 75,
          "column": 1,
          "byte": 2450
        },
        "end": {
          "line": 75,
          "column": 23,
          "byte": 2472
        }
      },
      "snippet": {
        "context": null,
        "code": "module \"account_label\" {",
        "start_line": 75,
        "highlight_start_offset": 0,
        "highlight_end_offset": 22,
        "values": []
      }
    },
    {
      "severity": "error",
      "summary": "Module not installed",
      "detail": "This module is not yet installed. Run \"terraform init\" to install all modules required by this configuration.",
      "range": {
        "filename": "providers.tf",
        "start": {
          "line": 20,
          "column": 1,
          "byte": 388
        },
        "end": {
          "line": 20,
          "column": 21,
          "byte": 408
        }
      },
      "snippet": {
        "context": null,
        "code": "module \"email_label\" {",
        "start_line": 20,
        "highlight_start_offset": 0,
        "highlight_end_offset": 20,
        "values": []
      }
    },
    {
      "severity": "error",
      "summary": "Module not installed",
      "detail": "This module is not yet installed. Run \"terraform init\" to install all modules required by this configuration.",
      "range": {
        "filename": "providers.tf",
        "start": {
          "line": 15,
          "column": 1,
          "byte": 285
        },
        "end": {
          "line": 15,
          "column": 19,
          "byte": 303
        }
      },
      "snippet": {
        "context": null,
        "code": "module \"iam_roles\" {",
        "start_line": 15,
        "highlight_start_offset": 0,
        "highlight_end_offset": 18,
        "values": []
      }
    },
    {
      "severity": "error",
      "summary": "Module not installed",
      "detail": "This module is not yet installed. Run \"terraform init\" to install all modules required by this configuration.",
      "range": {
        "filename": "context.tf",
        "start": {
          "line": 23,
          "column": 1,
          "byte": 918
        },
        "end": {
          "line": 23,
          "column": 14,
          "byte": 931
        }
      },
      "snippet": {
        "context": null,
        "code": "module \"this\" {",
        "start_line": 23,
        "highlight_start_offset": 0,
        "highlight_end_offset": 13,
        "values": []
      }
    }
  ]
}
terraform validate -json  0.12s user 0.05s system 141% cpu 0.124 total

Because it took only 0.12s, in case of a problem, that absolutely OK to do that check

I suppose it could be done using next or little-bit changed function:

function match_validate_errors {

That code is mostly the same as what we need to execute then

# terraform validate, plus capture possible errors
validate_output=$(terraform validate -json "${args[@]}" 2>&1)
exit_code=$?
# Match specific validation errors
local -i validate_errors_matched
match_validate_errors "$validate_output"
validate_errors_matched=$?
# Errors matched; Retry validation
if [ "$validate_errors_matched" -eq 1 ]; then
common::colorify "yellow" "Validation failed. Removing cached providers and modules from \"$dir_path/.terraform\" directory"
# `.terraform` dir may comprise some extra files, like `environment`
# which stores info about current TF workspace, so we can't just remove
# `.terraform` dir completely.
rm -rf .terraform/{modules,providers}/
common::colorify "yellow" "Re-validating: $dir_path"
common::terraform_init 'terraform validate' "$dir_path" || {
exit_code=$?
return $exit_code
}

As replace to

common::terraform_init 'terraform validate' "$dir_path" || {
exit_code=$?
return $exit_code
}

@smelchior
Copy link
Contributor Author

perfect, i will create a new PR and close this one afterwards :)

@smelchior
Copy link
Contributor Author

As discussed, I made the change in #524 in a "simple" way, i think this should be sufficient to achieve what we want :-)

@smelchior smelchior closed this May 15, 2023
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