Use the XDG Basedir Spec for the cache #208
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, for caching data, a new directory is created in the current user's home directory. This is bad practice (if all apps just dump files into the user's home, it becomes somewhat unmaintainable). The XDG Basedir Specification covers this pretty neatly; the cache directory for all applications is configurable via the `XDG_CACHE_HOME` variable, and falls back to `~/.cache` if unset. This changeset makes the cache for all applications live in one single location, rather that littering the user's home.
|
Hello @WhyNotHugo, thanks for sending this pull request! There are a few things that I think need to be addressed before we can merge a change like this:
If you're willing and able to make these changes, I'd be happy to move forward with this PR. If you're not, I'd be happy to leave this open and then use it as the base of a change that does address the other concerns. |
|
Hi! I was unaware that Mac used a different location (should have guessed it!). Thanks for pointing out that to already has a function that does the heavy lifting -- I'm very happy that an external lib is not needed.
I'll address these changes and update the PR. It'll take me a while since I'm very rusty in go, but it's a fair challenge! :)
Thanks!
… On Apr 20, 2020, at 22:50, Samuel Karp ***@***.***> wrote:
Hello @WhyNotHugo, thanks for sending this pull request! There are a few things that I think need to be addressed before we can merge a change like this:
This change as it stands is not backwards-compatible with installations where files are already present in ~/.ecr. The credential helper should either continue to use those files when present, or handle migrating them to the new location without intervention.
The XDG Base Directory Specification is widely used on Linux, but not so on other operating systems. The credential helper supports Linux, Windows, and macOS, and should work with the appropriate location for each. See golang/go#22536 for a discussion of the correct cache location, and golang/go#29960 for a discussion of config locations. The code as-is doesn't use the correct locations for Windows or macOS, but I would prefer to not migrate twice; we should either gate this change to be Linux-only or also move to the correct locations for Windows and macOS at the same time. Go 1.11 added os.UserCacheDir; I would be okay dropping support for Go 1.10 in order to use that function.
This change affects the log file location, which is documented in the README; that needs to be updated.
Since the file is moving, I think the new location should be documented in the README and the manual page.
If you're willing and able to make these changes, I'd be happy to move forward with this PR. If you're not, I'd be happy to leave this open and then use it as the base of a change that does address the other concerns.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
added 2 commits
May 17, 2020 20:31
This returns the per-platorm cache dir, since the XDG-Basedir is *nix-specific (except macOS?).
|
[Note: Still have pending moving existing files]
There seems to be no mention on the manual page right now. Should I add a FILES section at the bottom? |
You're right! It would be nice to add a FILES section, but I won't block accepting this PR on that change. |
samuelkarp
reviewed
Jun 24, 2020
| @@ -19,5 +19,7 @@ func GetCacheDir() string { | |||
| if cacheDir := os.Getenv("AWS_ECR_CACHE_DIR"); cacheDir != "" { | |||
| return cacheDir | |||
| } | |||
| return "~/.ecr" | |||
| dir, _ := os.UserCacheDir() | |||
There was a problem hiding this comment.
Let's not ignore the error here.
Suggested change
| dir, _ := os.UserCacheDir() | |
| dir, err := os.UserCacheDir() | |
| if err != nil { | |
| return "~/.ecr" | |
| } |
| @@ -214,7 +214,8 @@ There is no need to use `docker login` or `docker logout`. | |||
|
|
|||
| ## Troubleshooting | |||
|
|
|||
| Logs from the Amazon ECR Docker Credential Helper are stored in `~/.ecr/log`. | |||
| Logs from the Amazon ECR Docker Credential Helper are stored inside you cache | |||
| files directory (e.g.: `~/.cache/ecr/log` on *nix). | |||
There was a problem hiding this comment.
Let's either link to the Go documentation or describe the specific locations for Linux, Windows, and Mac.
3 tasks
|
Hey @WhyNotHugo! I've taken your commits and adjusted them to add fall-back behavior to |
|
Sorry, I never got around to finishing those changes (plus, my Happy that it's being picked up. I'm of course perfectly fine with the code being reused! Happy holidays! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, for caching data, a new directory is created in the current user's home directory. This is bad practice (if all apps just dump files into the user's home, it becomes somewhat unmaintainable).
The XDG Basedir Specification covers this pretty neatly; the cache directory for all applications is configurable via the
XDG_CACHE_HOMEvariable, and falls back to~/.cacheif unset.This PR makes the cache for all applications live in one single location, rather that littering the user's home.
Fixes #205
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.