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

Exclude terraform providers and .git repository when hashing workspace #197

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

toastwaffle
Copy link
Contributor

@toastwaffle toastwaffle commented Oct 4, 2023

Description of your changes

Excludes all files under .terraform/providers when computing the workspace checksum, to avoid reading and hashing large provider binaries when the plugin cache is disabled. When the plugin cache is enabled, the providers are stored outside of the workspace directory, and symlinked into the workspace. find does not follow symlinks by default, and so the providers are ignored for the checksum.

This also excludes the .git directory to avoid unnecessary terraform inits when repository objects change without affecting the working copy used. Fixes #198.

This will cause a (theoretically no-op) terraform init in each workspace after upgrading if the plugin cache is disabled or where git is used, as the hash will change due to no longer including the providers and/or .git directory.

Relates to #196 (but doesn't entirely fix it - that issue also explores the possibility of using PVCs)

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

kubectl execed into the provider-terraform pod and ran the find command; confirmed that only the provider binaries were excluded compared to the previous command.

@ytsarev
Copy link
Member

ytsarev commented Oct 6, 2023

/test-examples="examples/workspace-inline-aws.yaml"

@bobh66
Copy link
Collaborator

bobh66 commented Oct 13, 2023

@toastwaffle this change looks good to me - would you be willing to extend it to also exclude the .git directory? We had an issue opened from someone with a similar problem using a remote git module, where their tag/branch hasn't changed but the git fetch causes it to appear like there is a difference because of other unrelated changes in the repo.

@toastwaffle toastwaffle changed the title Exclude terraform providers when hashing workspace Exclude terraform providers and .git repository when hashing workspace Oct 16, 2023
@toastwaffle
Copy link
Contributor Author

@bobh66 I've updated to also exclude the .git directory. I've tested it locally with a simulated workspace directory containing a git repository, but we don't actually use git with provider-terraform.

@headyj could you kubectl exec into the provider-terraform pod, cd to a workspace (under /tf/ in the container) with a .git dir, and run /usr/bin/find . -path ./.git -prune -o -path ./.terraform/providers -prune -o -type f -exec /usr/bin/md5sum {} + to confirm that only relevant files are being hashed?

@headyj
Copy link

headyj commented Oct 16, 2023

@toastwaffle for what is see it seems OK. maybe we could also ignore .git-credentials file which is not related to terraform, but it should not change unlike .git directory does so the issue should be fix even without ignoring it.

@bobh66
Copy link
Collaborator

bobh66 commented Oct 16, 2023

@toastwaffle for what is see it seems OK. maybe we could also ignore .git-credentials file which is not related to terraform, but it should not change unlike .git directory does so the issue should be fix even without ignoring it.

I think it .git-credentials changes it is reasonable to rerun terraform init to make sure we have the latest content from git.

@toastwaffle
Copy link
Contributor Author

@bobh66 can we get this merged?

@ytsarev
Copy link
Member

ytsarev commented Oct 27, 2023

/test-examples="examples/workspace-inline-aws.yaml"

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

LGTM, I will let @bobh66 to do the final merge as he had the comments to this PR earlier

Copy link
Collaborator

@bobh66 bobh66 left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@bobh66 bobh66 merged commit 39d3807 into upbound:main Nov 7, 2023
10 checks passed
@headyj
Copy link

headyj commented Nov 28, 2023

@bobh66 any idea when this fix will be released? Still not in v0.11.0 release a month ago

@toastwaffle
Copy link
Contributor Author

toastwaffle commented Nov 28, 2023

Should be this Thursday; releases are scheduled for the last Thursday of each month.

See https://docs.upbound.io/providers/support

@toastwaffle toastwaffle deleted the md5sum branch November 28, 2023 09:24
@turkenf
Copy link
Contributor

turkenf commented Nov 28, 2023

Should be this Thursday; releases are scheduled for the last Thursday of each month.

@headyj, yes, we will cut a release containing this PR on Thursday, November 30.

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.

Cheksum is recalculated every time even without any change
5 participants