-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add configuration option for terraform plugin dir #787
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation LGTM, but would be nice to have an integration test. I think you can test this by doing the following:
- Take one of the terraform examples we have
- Create a temporary folder as the plugin dir
- Run
terraform init
without configuring plugin dir - Move the plugin folder from the created
.terraform
folder to the temporary path - Remove the
.terraform
folder - Run
terraform init
with the plugin dir configured to the temporary path
That sounds reasonable, let me give that a shot! thanks! |
@yorinasub17 I wrote up a test for it but its a bit weird since there's not much indication from the
Here is the output from the test:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test! It mostly looks good. Had one minor nit around cleanup, and also have a suggestion for the concern around verifiability:
Perhaps we can run InitE
with the PluginDir
configured but before copying the plugins, and verify that it errors? That way we know the -plugin-dir
is being passed given that it errors when the folder is empty, and passes when it is not. What do you think?
That's a great idea, definitely gives me more confidence that things are working as expected. Here is what the test looks like now:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have one more minor issue around cleanup, but other than that this LGTM! Will kick off the build after you address that and if that passes, we can merge this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM now! Will kick off a build and if it passes, we can merge this in!
Sorry for the delay in getting back to this. It looks like the tests are failing consistently, and only when run all together. I will have to investigate this to see what might be causing the concurrency issues, and will report back findings. |
Alrightly no problem, let me know if there's something I can help investigate! |
rebased with master. Let me know if there's anything I can help with @yorinasub17! |
@cbuto apologies for the delay in getting back to this. I got busy with other work initiatives and took a long time for me to come back. I finally got to the bottom of the test failures. In our CI environment we use terraform 0.14, and the plugin folder changed (from Can you update the test to attempt to copy over both |
@yorinasub17 No problem, I completely understand! Thanks for digging into this. I was always confused why terraform 0.13 called the directory I updated the test to copy over both directories. I wonder if I should attempt to check the terraform version before doing the copy? Anyway, the test passes now with both 0.13 and 0.14, let me know what you think:
|
Tests failed, but for an unrelated transient issue, so we can merge this in! |
Fixes #565.
I posed a question on the issue itself about testing but its probably better to have the conversation here:
I'm having a hard time figuring out the best way to implement a test that would be easy to maintain long term. Testing that the
-plugin-dir
option is actually working requires having pre-downloaded providers. I wrote a test that uses a fixture with a pre-downloaded provider locally but i don't think committing the actual provider binary to the repo is a viable solution.Do you have any suggestions on how to implement an actual test for this?
Thanks!