Feature/xdg base dir 2 #68

teddylear · 2021-06-20T04:13:19Z

Setting the 'PACKER_CONFIG_DIRandPACKER_CONFIG_DIR` to use the default XDG spec for unix systems. Not sure of the best way of testing these changes as they are operating system specific for the paths. Because of this I removed tests for cache directory as they changed depending on the operating system. If you have any advice for how to add tests for these please let me know!

Addresses: hashicorp/packer#9007

packer/cache_config_unix.go

SwampDragons · 2021-06-28T22:16:35Z

Hi, thanks for opening this. I haven't had a chance to test yet, but this appears like it would be backwards-breaking in that new versions of Packer will look in a new place and error if the XDG_CACHE_HOME env var exists, but the config file isn't there.

I think to preserve backwards compatability it would be better to first check for the config in the XDG_CACHE_HOME and then fall back to checking the old path if nothing is discovered in XDG_CACHE_HOME.

… in the xdg config dir

teddylear · 2021-06-29T02:04:45Z

@SwampDragons I updated PR to check for XDG_CONFIG_HOME env car and for packer configuration there before defaulting back to old location. I'm assuming for the cache it does not matter the location as it's a cache. Please let me know if there's anything I should change and if there is a way to automate the testing of this.

ogarcia · 2021-06-29T09:06:42Z

@teddylear bad update, XDG_CONFIG_HOME does not have to be configured or exist as environment variable. The behavior must be:

If XDG_CONFIG_HOME exists as environment variable use it
If not use default: $HOME/.config

I think that you must check if old config dir exists and in this case use it, if not, use XDG.

teddylear · 2021-06-29T22:52:26Z

@SwampDragons I just want to confirm the behavior that we want here. Do we want the behavior listed on the issue that @ogarcia is suggesting with:

If XDG_CONFIG_HOME exists as environment variable use it
If not use default: $HOME/.config

Or the behavior I believe your last comment suggests which is:

Check if XDG_CONFIG_HOME and if there is configuration there then use it
else use the default location we have already today.

I'm assuming for the cache we do not mind where this is placed as it's a cache. Please let me know how to proceed.

ogarcia · 2021-06-30T07:27:20Z

@teddylear you forgot one step:

Check if default location that we have today exists, if yes, use it (backwards compatibility).
Else If XDG_CONFIG_HOME exists as environment variable use it
Else use default: $HOME/.config

With this implementation, the new users have XDG, and old ones can delete default location or move current config to XDG to have it.

SwampDragons · 2021-06-30T18:02:32Z

I think either mine as you restated it, or ogarcia's work. I'd originally skipped the "check home, then xdg" because I figure if users have a packer config in the xdg-set location it's intentional and should be used, but I can see the benefit of keeping pure backwards compat.

ogarcia · 2021-07-01T06:55:35Z

@SwampDragons the main reason of perform check before is that all Linux/MacOS/Derivatives users are using XDG because is a standard (even if the XDG_CONFIG_HOME environment variable is not explicity declared).

In the other hand, using your approach for a user who was not configuration, if the app need to create something in its config directory will use default dir instead of XDG_CONFIG_HOME because XDG_CONFIG_HOME is empty for these user.

teddylear · 2021-07-02T17:10:44Z

I have updated with @ogarcia suggestion. Please let me know if there is anything to update or change.

ogarcia · 2021-07-03T10:49:01Z

@teddylear mutch better! But you must fix XDG_CACHE_HOME because now it have the logic:

If XDG_CACHE_HOME exists it will be used
Else, use a default packer_cache directory

And must be:

If XDG_CACHE_HOME exists as environment variable use it
If not use default: $HOME/.cache

Or use same logic that XDG_CONFIG_HOME if you want backwards compatibility, but is a cache and I think that is not needed in this case. 😉

teddylear · 2021-07-06T23:12:07Z

@ogarcia I believe I have updated with the desired logic.

ogarcia · 2021-07-07T07:03:13Z

packer/cache_config_unix.go

+		defaultConfigFileDir = filepath.Join(xdgCacheHome, "packer")
+	} else {
+		homeDir := os.Getenv("HOME")
+		defaultConfigFileDir = filepath.Join(homeDir, ".cache")


You miss add packer here.

defaultConfigFileDir = filepath.Join(homeDir, ".cache", "packer")

Updated for cache and config.

…>/packer

ogarcia · 2021-07-08T06:32:06Z

pathing/config_file_unix.go

+		log.Printf("Old default config directory found: %s", dir)
+	} else if xdgConfigHome := os.Getenv("XDG_CONFIG_HOME"); xdgConfigHome != "" {
+		log.Printf("Detected xdg config directory from env var: %s", xdgConfigHome)
+		dir = xdgConfigHome


Here is miss the packer too

dir = filepath.Join(xdgConfigHome, "packer")

…DG_CONFIG_HOME/packer

ogarcia · 2021-07-09T06:30:03Z

@teddylear Great work! IMHO it's ready to merge 😉

teddylear · 2021-07-14T23:49:36Z

@SwampDragons This is ready to review, please let me know if you have any ideas around automating tests for this.

SwampDragons · 2021-07-20T17:41:59Z

sorry, I'm slammed. I'll try to take a look soon.

azr · 2021-07-21T13:44:47Z

packer/cache.go

-//
-// CachePath can error in case it cannot find the cwd.
+// When the directory is not absolute, CachePath will try to make a
+// a cache depending on the operating system.
 //


👋🏼
Could you please update the examples down here in this list depending on the OS ? That would make it easier for me. 🙂

Super awesome, thanks.

packer/cache.go

azr · 2021-07-21T13:48:23Z

packer/cache_test.go

-	"testing"
-)
-
-func TestCachePath(t *testing.T) {


Why removing the tests ? I think it would be better to update them.

As mentioned above, I'm having issues with creating tests with each path being different depending on the OS. If you have any examples of how to do this I would be glad to update as I tried to update the tests in place but hit a wall making them work for without needing to know the OS.

Because the behaviors differ between OS, you will have to create tests that test things under unix and under windows. One way to fix this would be to check using runtime.GOOS == "windows", may be the table tests could be different depending on the os type ?

Updated to make different test files depending on OS similar to the OS specific cache and config files.

Super good ! Thanks.

pathing/config_file_unix.go

pathing/config_file_windows.go

packer/cache_config_unix.go

Co-authored-by: Adrien Delorme <azr@users.noreply.github.com>

comments. Adding comment for how the cache configuration works.

teddylear · 2021-07-28T01:37:16Z

@azr Addressed previous comments. Please let me know if there is anything else to updates.

azr · 2021-07-28T08:56:09Z

packer/cache_config_unix_test.go

+	// reset env
+	packerCacheDir := os.Getenv("PACKER_CACHE_DIR")
+	os.Setenv("PACKER_CACHE_DIR", "")
+	defer func() {
+		os.Setenv("PACKER_CACHE_DIR", packerCacheDir)
+	}()
+
+	xdgCacheHomeDir := os.Getenv("XDG_CACHE_HOME")
+	os.Setenv("XDG_CACHE_HOME", "")
+	defer func() {
+		os.Setenv("XDG_CACHE_HOME", xdgCacheHomeDir)
+	}()


Super nice !

azr · 2021-07-28T09:03:43Z

packer/cache.go

+// For Unix:
+//   PACKER_CACHE_DIR="",            XDG_CONFIG_HOME="", Default_config CacheDir() => "$HOME/cache/packer"
+//   PACKER_CACHE_DIR="",            XDG_CONFIG_HOME="", Default_config CacheDir("foo") => "$HOME/cache/packer/foo"
+//   PACKER_CACHE_DIR="bar",         XDG_CONFIG_HOME="", Default_config CacheDir("foo") => "./bar/foo
+//   PACKER_CACHE_DIR="/home/there", XDG_CONFIG_HOME="", Default_config CacheDir("foo", "bar") => "/home/there/foo/bar


I like these new examples !
What will Default_config stand for here ?
Do you think we should add something for XDG_CONFIG_HOME too ?

Suggested change

// For Unix:

// PACKER_CACHE_DIR="", XDG_CONFIG_HOME="", Default_config CacheDir() => "$HOME/cache/packer"

// PACKER_CACHE_DIR="", XDG_CONFIG_HOME="", Default_config CacheDir("foo") => "$HOME/cache/packer/foo"

// PACKER_CACHE_DIR="bar", XDG_CONFIG_HOME="", Default_config CacheDir("foo") => "./bar/foo

// PACKER_CACHE_DIR="/home/there", XDG_CONFIG_HOME="", Default_config CacheDir("foo", "bar") => "/home/there/foo/bar

// For Unix:

// PACKER_CACHE_DIR="", XDG_CONFIG_HOME="", Default_config CacheDir() => "$HOME/cache/packer"

// PACKER_CACHE_DIR="", XDG_CONFIG_HOME="", Default_config CacheDir("foo") => "$HOME/cache/packer/foo"

// PACKER_CACHE_DIR="bar", XDG_CONFIG_HOME="", Default_config CacheDir("foo") => "./bar/foo

// PACKER_CACHE_DIR="/home/there", XDG_CONFIG_HOME="", Default_config CacheDir("foo", "bar") => "/home/there/foo/bar

// PACKER_CACHE_DIR="", XDG_CONFIG_HOME="/home/there", Default_config CacheDir("foo", "bar") => "/home/there/foo/bar

azr · 2021-07-28T09:22:52Z

Super awesome, thanks for bearing with us @teddylear, I just want to say that this LGTM in terms of code and I just had a question/suggestion on docs.

SwampDragons · 2021-07-28T17:50:44Z

Thanks for this!

teddylear force-pushed the feature/xdg-base-dir-2 branch 2 times, most recently from e699b7b to 4ae7de2 Compare June 24, 2021 22:32

teddylear closed this Jun 24, 2021

teddylear reopened this Jun 24, 2021

teddylear force-pushed the feature/xdg-base-dir-2 branch from 4ae7de2 to ce184f9 Compare June 24, 2021 22:57

teddylear marked this pull request as ready for review June 24, 2021 22:59

teddylear requested a review from a team as a code owner June 24, 2021 22:59

teddylear closed this Jun 24, 2021

teddylear reopened this Jun 24, 2021

Updating default config and cache directory for unix systems

f8d8c28

teddylear force-pushed the feature/xdg-base-dir-2 branch from ce184f9 to f8d8c28 Compare June 24, 2021 23:01

ogarcia suggested changes Jun 25, 2021

View reviewed changes

packer/cache_config_unix.go Outdated Show resolved Hide resolved

Fixing cache config for unix to point to XDG_CACHE_HOME

50d37c5

teddylear added 2 commits June 28, 2021 20:44

Changing XDG config dir to be used for packer config dir if it exists…

0155950

… in the xdg config dir

Updating to make point to proper function

2d18051

teddylear added 3 commits July 2, 2021 12:54

Fixing config direcotry function to match new specs

e436960

Fixing linting issues

1fae0aa

Adding imports to config file windows

655a261

Update cache directory configuration logic

beaa322

ogarcia reviewed Jul 7, 2021

View reviewed changes

Updated last case for cache and config to be $HOME/<.cache or .config…

1aee4ae

…>/packer

ogarcia reviewed Jul 8, 2021

View reviewed changes

Updating config file directory when $XDG_CONFIG_HOME defined to be $X…

60ef4e6

…DG_CONFIG_HOME/packer

azr reviewed Jul 21, 2021

View reviewed changes

packer/cache.go Outdated Show resolved Hide resolved

azr reviewed Jul 21, 2021

View reviewed changes

pathing/config_file_unix.go Outdated Show resolved Hide resolved

azr reviewed Jul 21, 2021

View reviewed changes

pathing/config_file_windows.go Outdated Show resolved Hide resolved

azr reviewed Jul 21, 2021

View reviewed changes

packer/cache_config_unix.go Outdated Show resolved Hide resolved

teddylear and others added 3 commits July 22, 2021 13:16

Update pathing/config_file_windows.go

281eb19

Co-authored-by: Adrien Delorme <azr@users.noreply.github.com>

Update packer/cache_config_unix.go

31918db

Co-authored-by: Adrien Delorme <azr@users.noreply.github.com>

Add tests for changing config and cache configuration. Addressing PR

006be29

comments. Adding comment for how the cache configuration works.

azr reviewed Jul 28, 2021

View reviewed changes

sylviamoss assigned sylviamoss and SwampDragons and unassigned sylviamoss Jul 28, 2021

SwampDragons merged commit 4c9e709 into hashicorp:main Jul 28, 2021

teddylear mentioned this pull request Jul 28, 2021

Updating comments for cache and config directories to be accurate #73

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/xdg base dir 2 #68

Feature/xdg base dir 2 #68

teddylear commented Jun 20, 2021 •

edited

Loading

SwampDragons commented Jun 28, 2021

teddylear commented Jun 29, 2021

ogarcia commented Jun 29, 2021

teddylear commented Jun 29, 2021

ogarcia commented Jun 30, 2021

SwampDragons commented Jun 30, 2021

ogarcia commented Jul 1, 2021

teddylear commented Jul 2, 2021

ogarcia commented Jul 3, 2021

teddylear commented Jul 6, 2021

ogarcia Jul 7, 2021

teddylear Jul 7, 2021

ogarcia Jul 8, 2021

teddylear Jul 8, 2021

ogarcia commented Jul 9, 2021

teddylear commented Jul 14, 2021

SwampDragons commented Jul 20, 2021

azr Jul 21, 2021

teddylear Jul 28, 2021

azr Jul 28, 2021

azr Jul 21, 2021 •

edited

Loading

teddylear Jul 22, 2021

azr Jul 27, 2021 •

edited

Loading

teddylear Jul 28, 2021

azr Jul 28, 2021

teddylear commented Jul 28, 2021

azr Jul 28, 2021

azr Jul 28, 2021

azr commented Jul 28, 2021

SwampDragons commented Jul 28, 2021

Feature/xdg base dir 2 #68

Feature/xdg base dir 2 #68

Conversation

teddylear commented Jun 20, 2021 • edited Loading

SwampDragons commented Jun 28, 2021

teddylear commented Jun 29, 2021

ogarcia commented Jun 29, 2021

teddylear commented Jun 29, 2021

ogarcia commented Jun 30, 2021

SwampDragons commented Jun 30, 2021

ogarcia commented Jul 1, 2021

teddylear commented Jul 2, 2021

ogarcia commented Jul 3, 2021

teddylear commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogarcia commented Jul 9, 2021

teddylear commented Jul 14, 2021

SwampDragons commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azr Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azr Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teddylear commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azr commented Jul 28, 2021

SwampDragons commented Jul 28, 2021

teddylear commented Jun 20, 2021 •

edited

Loading

azr Jul 21, 2021 •

edited

Loading

azr Jul 27, 2021 •

edited

Loading