Add method for retrieving XDG config directory #2211
Conversation
There was a problem hiding this comment.
Your point on Windows is a fair one. From what I found, people advocate for a %LOCALAPPDATA%/Vendor/App/<tempLocation>. We should probably also allow checking for settings in their roaming profile in %APPDATA% too.
And the macOS cache directory really should be Library/Caches. Plus we're exporting our org structure by using google-cloud-tools-java, since Chrome uses Google/Chrome (on macOS at least).
jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/XdgDirectories.java
Outdated
Show resolved
Hide resolved
| String rawOsName = properties.getProperty("os.name"); | ||
| String osName = rawOsName.toLowerCase(Locale.ENGLISH); | ||
|
|
||
| if (osName.contains("linux")) { |
There was a problem hiding this comment.
We're now duplicating some of this platform-testing logic. Is it worth extracting into separate getOs(properties) method that returns the platform type? (I wish the JRE already provided this.)
There was a problem hiding this comment.
Might be a good idea if we ever need it more. But I think with how little logic it actually is, only duplicating in 2 places isn't so bad.
Yeah, I wonder if we should think about a way of moving the cache without breaking users. |
|
I don't think we'd break anyone except those that have some code to auto clear cache. We could do something like: Identify if an old cache exist -- tell the user to delete it If we do think we should do this, we should do it before 2.0.0 |
|
Filed #2216 |
|
Since we're thinking about changing the cache directory on different platforms, I'll update this PR so the |
| * | ||
| * <p>For Linux, this is {@code $HOME/.config/google-cloud-tools-java/jib/}. | ||
| * | ||
| * <p>For Windows, this is {@code %LOCALAPPDATA%\Google\Jib/}. |
There was a problem hiding this comment.
...\Google\Jib\ (instead of the trailing /)
| */ | ||
| @VisibleForTesting | ||
| static Path getConfigHome(Properties properties, Map<String, String> environment) { | ||
| Path windowsSubDirectory = JIB_SUBDIRECTORY_OTHER.resolve("Config"); |
There was a problem hiding this comment.
Why not having windowsSubDirectory as a constant?
There was a problem hiding this comment.
It's different for cache and config.
| LOGGER.warning(applicationSupport + " does not exist"); | ||
| return xdgPath.resolve(JIB_SUBDIRECTORY_OTHER); | ||
| } | ||
| return applicationSupport.resolve(JIB_SUBDIRECTORY_OTHER); |
There was a problem hiding this comment.
Looks like this structure is being repeated.
if (linux) {
return path.resolve(JIB_SUBDIRECTORY_LINUX);
} else if (windows) {
return path.resolve(windowsSubDirectory);
} else /* if (mac) */ {
return path.resolve(JIB_SUBDIRECTORY_OTHER);
}
Perhaps this logic can be a separate method, like getConfigHome(getBaseConfigHome(...)) (whatever suitable method names)? If this is indeed repeated, I think factoring this out will make the code more readable.
There was a problem hiding this comment.
Sure, I'll give it a try.
|
Closing in favor of #2218 |
Part of #2193. Adds methods for retrieving the XDG_CONFIG_HOME directory, which will hold update-check-related files.
The full paths are:
$HOME/.config/google-cloud-tools-java/jib/$HOME/Library/Preferences/Google/Jib/%LOCALAPPDATA%\Google\Jib\ConfigOne potential issue is that this may end up overlapping with the base image cache directory on windows, so if someone clears their cache without being careful, they may also delete their update check config accidentally.