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

Require terraform-docs runs in serial #33

Merged

Conversation

chrisgilmerproj
Copy link
Contributor

It turns out that pre-commit defaults to running hooks in parallel. This means it will take a batch of files, break them into chunks, and run those together. The terraform_docs.sh script strips the filenames and runs over a set of unique paths. That means that parallel operations can be running int he same directories. To avoid pre-commit doing parallel operations on similar file paths I've added require_serial: true to the hook descriptions.

For some detail, I ran into this while running on docker. I copied a large terraform directory into a docker container and then ran the terraform-docs pre-commit hook to mimic our CI environment in CircleCI. In about 10-20% of cases the hook would fail and the result would be that one or more files were deleted. It turns out the chunking from pre-commit is non-deterministic, so sometimes similar directories would be groups and sometimes not. And when they were not grouped we had a race condition where perl would be operating on an in-place file operation when the underlying file would get removed.

I attempted to revert to using sed and had less failures but it would happen, just less often. I think the combination of perl and docker is what made this show up because perl takes just a bit longer than sed and docker is resource constrained enough on my mac to cause the race condition to bite me.

Fortunately this is a one-line fix and users who are currently experiencing this can immediately fix it by adding the same line, per this example:

  - repo: git://github.com/antonbabenko/pre-commit-terraform
    rev: v1.8.1
    hooks:
      - id: terraform_docs
        require_serial: true

@chrisgilmerproj
Copy link
Contributor Author

@antonbabenko - Can I do anything for you to help move this along?

@antonbabenko
Copy link
Owner

Thanks for opening this issue! I will merge it right away.

Unfortunately (or luckily, lol) I have not seen this bug before because codebase I work with is usually pretty small.

Just curious, what size of your code which reveals this problem?

@antonbabenko antonbabenko merged commit 1750003 into antonbabenko:master Feb 18, 2019
@antonbabenko
Copy link
Owner

v1.9.0 has been released.

PS: We now have CHANGELOG.md also !!! 🎉

@chrisgilmerproj
Copy link
Contributor Author

Thanks for responding so quickly!

We have 68 *.tf files split into 20 directories. On my machine it appears pre-commit chunks the files up into groups of 11 files but the order is random. So if two groups of files share a directory then you'll see the behavior where perl has the race condition to modify files.

@chrisgilmerproj chrisgilmerproj deleted the run_terraform_docs_in_serial branch February 18, 2019 18:03
@antonbabenko
Copy link
Owner

This does not sound like a lot of files. Good that it helps.

I wonder if #31 will be fast enough comparing to sed.

@chrisgilmerproj
Copy link
Contributor Author

I'd be curious to know if it's fast enough. My guess is that you'll still have a race condition that will pop up from time to time.

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.

2 participants