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

[WIP] #15389 use xdg basedir spec #21915

Closed
wants to merge 6 commits into from
Closed

[WIP] #15389 use xdg basedir spec #21915

wants to merge 6 commits into from

Conversation

06kellyjac
Copy link

This PR aims to resolve #15389

This is my first PR to terraform, I've had a good read through the README.md and CONTRIBUTING.md but if I've missed anything please shout.
Also I haven't written too much golang so any and all feedback/criticism is welcome, please feel free to be picky/strict

Note: Bellow I will explain the flow of priorities for env variables and directory defaults etc. I'm generally happy with it, but I'm also not against swapping the XDG_... env variables to have less priority than the "legacy" locations, as some people may have XDG_ env variables defined but still expect it to use their current .terraform.d .terraformrc files.


The existing flow for cliConfigFile() is:
TF_CLI_CONFIG_FILE then TERRAFORM_CONFIG then ConfigFile() where ConfigFile() results in ~/.terraformrc

Now it is TF_CLI_CONFIG_FILE then TERRAFORM_CONFIG then ConfigFile() where ConfigFile() results in ~/.terraformrc if it already exists but will use $XDG_CONFIG_HOME/terraform/.terraformrc or ~/config/terraform/.terraformrc if it doesn't.
(I can change it to ~/config/terraform/ for ConfigDir() and ~/config/.terraformrc for ConfigFile() if this may cause issues with some of the stuff around line 162 of config.go)
This allows for all previous setups to work just fine but default to XDG going forward (Although see the note in the first section). ~/.terraform.d can also gradually be fazed out if we want.


The existing flow for ConfigDir() is:
Just results ~/.terraform.d/

Now it is ~/.terraform.d/ if it already exists but will use $XDG_CONFIG_HOME/terraform/ or ~/config/terraform/ if it doesn't.
(I can change it to ~/config/terraform/ for ConfigDir() and ~/config/.terraformrc for ConfigFile() if this may cause issues with some of the stuff around line 162 of config.go)
This allows for all previous setups to work just fine going but default to XDG going forward (Although see the note in the first section). ~/.terraform.d can also gradually be fazed out if we want.


runCheckpoint() uses currently uses ConfigFile() for checkpoint_cache & checkpoint_signature but it should be outputting to XDG_CACHE_HOME
As far as I can tell they're completely safe to delete with one being called cache and the other saying within the file "you can delete this file at any time". If checkpoint_signature should be put in XDG_DATA_HOME instead please let me know.


One case where these changes may cause issues is scripts that utilize terraform and allow it to create a ~/terraform.d directory, then mess with it or whatever (e.g. maybe installing a plugin then the script doing something with ~/.terraform.d/plugins/PLUGIN-NAME-HERE or any of the checkpoint files: checkpoint_cache & checkpoint_signature)


Along the way there was quite a bit of mess. As far as I can tell .terraform.d was called a config dir but it didn't actually hold any config, ~/.terraformrc was the actual config file and only form of config.
Also there is XDG standard use in install.go for fishConfigDir().


Still TODO:

  • Update Docs references to ~/.terraform.d including PLUGIN_CACHE_DIR config examples
  • More thorough manual testing
  • Probably write some extra tests
  • Double check this doesn't break Windows

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 27, 2019

CLA assistant check
All committers have signed the CLA.

@06kellyjac
Copy link
Author

I added a fix to only keep the dot in-front of terraformrc when in a legacy location.
In ~/.config/terraform/ and/or $XDG_CONFIG_HOME/terraform/ it is undesirable for the file to be hidden

@mildwonkey
Copy link
Contributor

Hi! Thanks for submitting this. I'm trying to clean up some older PRs, so I will be closing this WIP PR. We have not forgotten about #15389 and have some design work for the future (unfortunately it's not on the immediate roadmap) that we need to work out before making this change. Thanks!

@mildwonkey mildwonkey closed this Oct 1, 2020
@06kellyjac
Copy link
Author

Hey @mildwonkey shall I update this for master, or just leave it to the TF team?

@mildwonkey
Copy link
Contributor

@06kellyjac honestly that's a tricky question! I don't want to tell you not to work on something you are interested in, but I don't want to waste your time but suggesting you work on something we're not likely to look at anytime soon, either.

We would like to change the configuration file format while we're working in the area and have a (currently vague and unscheduled) placeholder on our internal roadmap for this project. We certainly wouldn't be unhappy if you gave this PR another go, but I can't promise that we'll be able to look at it. My biggest concern, to be honest, is that we'd get started on the bigger project, forget about this PR, and do the same work on our own. That's not fair to you and it's not how we want to treat our community, but it does happen.

I'm sorry that I'm doing everything except answer your question! We really do appreciate you working on this, and would love to review an updated PR, but our focus is elsewhere and it's not very likely that we would be able to give this the attention it deserves.

I am, again, very sorry for the unsatisfying response, and thank you so much for working on this in the first place.

@ghost
Copy link

ghost commented Nov 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use xdg basedir spec on linux
4 participants