Always invoke terraform get #591
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
| cd $(BUILD_DIR) && $(TF_CMD) apply $(TOP_DIR)/platforms/$(PLATFORM) | ||
|
|
||
| destroy: installer-env ${BUILD_DIR}/.terraform | ||
| destroy: installer-env terraform-get |
There was a problem hiding this comment.
Are destroy, apply, & plan supposed to be phony? In other words: do you want them to run if a file named destroy is in the current dir?
There was a problem hiding this comment.
Will my change make destroy, apply and plan phony? I'm not a Make expert, but I figured that only the terraform-get target would become phony?
There was a problem hiding this comment.
I just tested creating such files FWIW (destroy/apply/plan), it seems the targets are executed no matter what, even if they exist. I would expect Make to skip them since corresponding files exist. This behaviour is the same without my patch.
There was a problem hiding this comment.
It's always building because its dependencies also have missing outputs. I think your PR is better than what's currently in master, so I don't think it should be blocked on this.
I just didn't realize the Makefile was so hacky.
There was a problem hiding this comment.
Haha thanks for clarifying this @ggreer :) I guess these targets should be phony.
There was a problem hiding this comment.
Can you please give a bit of background on why this change is needed?
What is your use-case where you need to run get before every apply?
The current behaviour is to invoke get only when module cache is missing.
There was a problem hiding this comment.
@alexsomesan The case is when the cache is missing one or more modules. I don't know how it came about, but I imagine it's because modules were added in the source, so that the cache became stale. It's only logical however that the cache can become out of sync with the source, we can't expect that it will stay relevant over time.
There was a problem hiding this comment.
You're right that a new module being added would not trigger a get. In that sense, your change is brings more convenience. There is also a downside because this is invisible to the user and could "bite" unsuspecting users into applying changes that they weren't expecting.
However, since the cache is nothing more than a bunch of symlinks to the actual module files, changes to those could also bite in the same way with the current state of things.
Things being the way they are, I think your change is useful and won't surprise with unexpected side-effects.
Please rebase and it should be good to merge.
|
Thanks @alexsomesan, I've rebased. |
Makefile
Outdated
| @@ -21,16 +21,17 @@ installer-env: $(INSTALLER_BIN) terraformrc.example | |||
| localconfig: | |||
| mkdir -p $(BUILD_DIR) | |||
|
|
|||
| $(BUILD_DIR)/.terraform: | |||
| .PHONY: terraform-get | |||
There was a problem hiding this comment.
One more thing: there already is a .PHONY section (at the end of the file).
Could you please move this over there?
It makes spotting target blocks easier when reading the Makefile.
|
Thanks! I forgot one thing: see comment about the .PHONY location. |
|
@alexsomesan Moved the phony declaration! |
|
ok to test |
* Initial VMware Support * Always invoke terraform get to ensure we have the necessary modules (#591) * Makefile: add vmware docs-examples * Run make docs for VMware updates To address; #639 (comment) * Run terraform fmt on VMware content run `terraform fmt ./platforms/vmware/` , `terraform fmt ./modules/vmware/etcd` , `terraform fmt ./platforms/vmware/nodes` to address #639 (comment) * Variables cleanup and change ssh variable - cleanup variables.tf and run `make examples` to address; #639 (comment) - run `make docs` - update `tectonic_ssh_authorized_key` to `tectonic_vmware_ssh_authorized_key ` to address #639 (comment) * Fix Makefile conflict with vmware branch * vmware: remove redundant bootkube content_hash variable Related to #575 * vmware: remove username secrets from variables.tf Removes VMware Username and Password variables from variables.tf. User will be prompted to enter credentials at terraform execution time, such as. ``` provider.vsphere.password The user password for vSphere API operations. Enter a value: provider.vsphere.user The user name for vSphere API operations. Enter a value: ``` * VMware: fix Master CPU/memory vars issue Master Node provisioning was utilizing worker CPU and memory variables. * vmware examples - re-run make docs examples * vmware: compute service k8s IPs Add changes in #767 for VMware * vmware: Replace all occurences of path.cwd Applies d3e0a6a into vmware platform * vmware: Bump bootkube v0.4.4 & hyperkube v1.6.4_coreos.0 Applies requirements for 5941fda on VMware Platform * vmware: run terraform fmt
Always invoke terraform get in order to ensure that necessary modules are installed, to avoid build errors, rather than relying on whether the .terraform directory exists, since it might be stale.
See #571 for reference.