ADR-22: Client file locations #88

philpennock · 2022-01-24T22:47:09Z

In discussion, it became clear that folks are more comfortable using a common directory holding things if there's some way to avoid multiple tools claiming the same files. There was also debate about how to manage migration safely, how to manage Windows, etc. I volunteered to write an ADR documenting a proposal.

At heart, we're using XDG on Unix with the https://github.com/adrg/xdg rules for mapping these locations to "native" locations on macOS and Windows.

This ADR has a proposal as a stake in the ground which folks can evaluate.

(Also, fixed the ADR template and parser to be a little more consistent, after I went wrong while following the template).

ripienaar

LGTM

ripienaar · 2022-01-25T07:23:14Z

main.go

@@ -28,7 +28,7 @@ type ADR struct {
 }

 var (
-	validStatus = []string{"Approved", "Partially Implemented", "Implemented"}
+	validStatus = []string{"Approved", "Partially Implemented", "Implemented", "Proposed"}


An unmerged PR is defacto Proposed and a merged one at least Approved. There shouldn’t really be proposed and merged ones.

So we tend to put the aspirational value - Approved in this case or Partially Implemented given the nats cli - during PR time so once merged it’s correct.

Ah. Then since the README says to run this, I guess the fix is to change the README and the template text, then revert the Go change?

No doubt the README can be improbed you're not the first that made this error - happy to see edits there to improve it

ripienaar · 2022-01-25T09:13:16Z

adr/ADR-22.md

+For the time being, we instead simply mandate that files be accessible by the
+new paths; it is acceptable for client libraries to detect that the new
+location does not exist on disk but the old location does exist, and put in a
+symbolic link at the new location pointing to the old location.


Granted I have only like 10 apps using ~/.config so not an exhaustive set but I dont see any that make such symlinks - and I know I use several that support XDG but ignore it if old files are found. Is there existing example of such symlinks (not against them, just curious)

I think the difference is in whether or not the app considers the location something which has to be shared with other apps. If it's an internal detail, then just using the old location is fine. But if we're setting expectations for where all clients, across implementation languages, should look, then ensuring that "the required location" has a symlink to "the old location" removes the need for every client library implementation to know about historical variants to just work.

@philpennock can you clarify:

additionally require that ~/.config/nats work, but in new installs it should
be a symbolic link to the ~/Library/Application Support/nats directory.

But what are we doing for windows? Are we doing to then honor what the library says? Note that this is awesome, but it is somewhat fastidious, as we LGTM'ed to use the library which does the correct thing for in any OS (the addition of the symlink is fine I think) but the trick is it was rejected - I have since completed, and released, and this will do additional tweaks - and in the case of windows will put things completely elsewhere.

aricart · 2022-01-26T00:50:07Z

adr/ADR-22.md

+
+* [ADR-21][] on Configuration Contexts moves us closer to the model
+  described herein, but ADR-21 will need updating for the non-XDG platforms.
+* The `nsc` and `ngs` commands use other locations, scattering files around.


ngs tool doesn't write any state file (pretty sure)
nats cli is the only other cli tool

aricart · 2022-01-26T00:55:50Z

it would be great if @derekcollison could comment on this. As I understand as I understand his comments (he can elaborate) most unix tools don't use these locations. On my environment the only clis that I can find in OS X that store under ~/Library/Application Support are termshark and mkcert all other tools have an UI type component with the exception of the demon/services in OS X.

Fix the examples in the template to match the form actually needed by the parser, and remove the invalid `Proposed` status. Document the flow for new ADRs starting in `Accepted` status, to reflect that as soon as they've been merged they are Accepted.

This documents a combination of existing practice of some clients and an optimal New World Order to better support Windows, to try to get something well documented for all clients to adhere to.

philpennock · 2022-02-03T00:10:00Z

I have reverted the main.go changes and adjusted the README and template, per @ripienaar's feedback above.

In a meeting with @derekcollison, @aricart and @scottf we settled on the approach documented now: we use the adrg/xdg locations, because particularly on Windows the XDG locations do not work. For darwin/macOS we compromise and want both the XDG location and the system-native paths to work. Tools should mostly use the system native path, the .config/nats works better for humans exploring. We think.

Suggestions on better phrasing welcome.

ripienaar · 2022-02-03T07:49:02Z

adr/ADR-22.md

+    - Most tools should not need to be specifically aware in code of the
+      location of these; where credentials are passed in, they are typically
+      part of the site configuration.
+    - New tools should use [ADR-21][] configuration contexts to locate


fyi this repo will change ADR-21 to a link, so you can take away the markdown reference etc

aricart

LGTM - but generally want some discussion that is explicit for windows.

aricart · 2022-02-03T15:06:10Z

adr/ADR-22.md

+For the time being, we instead simply mandate that files be accessible by the
+new paths; it is acceptable for client libraries to detect that the new
+location does not exist on disk but the old location does exist, and put in a
+symbolic link at the new location pointing to the old location.


@philpennock can you clarify:

additionally require that ~/.config/nats work, but in new installs it should
be a symbolic link to the ~/Library/Application Support/nats directory.

But what are we doing for windows? Are we doing to then honor what the library says? Note that this is awesome, but it is somewhat fastidious, as we LGTM'ed to use the library which does the correct thing for in any OS (the addition of the symlink is fine I think) but the trick is it was rejected - I have since completed, and released, and this will do additional tweaks - and in the case of windows will put things completely elsewhere.

aricart · 2022-02-03T15:06:50Z

adr/ADR-22.md

+### The `nsc` CLI
+
+The default locations of files for `nsc` can be overridden with the
+environment variables: `$NSC_HOME`, `$NKEYS_PATH`, and `$NSC_CWD_ONLY`.


$NSC_CWD_ONLY shouldn't be part of this - since that enables a feature that allows multiple terminal instances to have the right context in terms of the /Operator/Account model. This is an app feature, not some standard.

aricart · 2022-02-03T15:08:13Z

adr/ADR-22.md

+In addition, `nsc` has traditionally detected an account system Operator in
+the current directory and then ignored all environment variables to use the
+current directory as a root.
+This behavior is considered an authentication administrator mode and should


Correct this is user - the reality is the data locations on a local user's directory is not the intended idea - The JWT configurations managed by nsc are probably shared between multiple groups - ie - should be in github.

philpennock requested review from ripienaar and aricart January 24, 2022 22:48

philpennock marked this pull request as ready for review January 24, 2022 22:48

philpennock mentioned this pull request Jan 24, 2022

Adopt XDG installation layout nats-io/nsc#445

Closed

ripienaar approved these changes Jan 25, 2022

View reviewed changes

aricart reviewed Jan 26, 2022

View reviewed changes

philpennock added 2 commits February 2, 2022 19:04

Propose ADR-22: Client File Locations

d5dce7d

This documents a combination of existing practice of some clients and an optimal New World Order to better support Windows, to try to get something well documented for all clients to adhere to.

philpennock force-pushed the client-file-locations branch from 3d771a4 to d5dce7d Compare February 3, 2022 00:07

ripienaar reviewed Feb 3, 2022

View reviewed changes

aricart approved these changes Feb 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR-22: Client file locations #88

ADR-22: Client file locations #88

philpennock commented Jan 24, 2022

ripienaar left a comment

ripienaar Jan 25, 2022

philpennock Jan 26, 2022

ripienaar Jan 26, 2022

ripienaar Jan 25, 2022 •

edited

Loading

philpennock Jan 26, 2022

aricart Feb 3, 2022

aricart Jan 26, 2022

aricart commented Jan 26, 2022

philpennock commented Feb 3, 2022

ripienaar Feb 3, 2022

aricart left a comment

aricart Feb 3, 2022

aricart Feb 3, 2022

aricart Feb 3, 2022

ADR-22: Client file locations #88

Are you sure you want to change the base?

ADR-22: Client file locations #88

Conversation

philpennock commented Jan 24, 2022

ripienaar left a comment

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

ripienaar Jan 25, 2022 • edited Loading

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

aricart commented Jan 26, 2022

philpennock commented Feb 3, 2022

Choose a reason for hiding this comment

aricart left a comment

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

ripienaar Jan 25, 2022 •

edited

Loading