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

Clarify directory for additional storage profiles #6322

Merged
merged 4 commits into from
Jun 21, 2023

Conversation

d108
Copy link
Contributor

@d108 d108 commented Mar 3, 2023

First time contributor checklist:

Contributor checklist:

  • My contribution is not related to translations.
  • My commits are in nice logical chunks with good commit messages
  • My changes are rebased on the latest main branch
  • A yarn ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

Description

The existing instructions help us to create multiple additional storage profiles with NODE_APP_INSTANCE. However, it was not immediately clear where to find the config directory, as there is a config.json in userData, and for Linux the data directory is in .config. Therefore, some minor adjustments can clarify potential points of confusion.

@scottnonnenberg-signal
Copy link
Contributor

With your changes, the documentation now references three terms: NODE_CONFIG_DIR ,appData and userData

We should probably define each of these, at a minimum linking to their definitions in the Electron docs.

But ideally we should only mention these concepts if they are really required. Can we get away with just mentioning appData?

@@ -150,7 +150,7 @@ Once you have the additional numbers, you can setup additional storage profiles
between them using the `NODE_APP_INSTANCE` environment variable.

For example, to create an 'alice' profile, put a file called `local-alice.json` in the
`config` directory:
`config` directory, a child of the Signal directory, not `appData` from above:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, the appData directory is the same as the config directory listed here. Have you seen differently? Maybe we could let go of that config term?

Copy link
Contributor Author

@d108 d108 Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On macOS, we have "~/Library/Application Support/Signal-development" for appData, whereas config corresponds to something like "/Volumes/development/Signal-Desktop/config." So the location for the JSON files ends up being different from appData. Therefore, removing the config term as a directory name wouldn't show the difference on macOS.

I had the problem thinking config was in "Application Support" when it appeared in the Signal directory instead. If the documentation changes don't work for all platforms, we can discuss how to make them more inclusive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you are right - I'm mistaken. Maybe it's best to call that something like 'the /config subdirectory of your project checkout?' We could potentially clarify that it should go alongside all the other .json config files there?

Copy link
Contributor Author

@d108 d108 Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, what you suggest helps clarify the location for where to put the .json config files.

@scottnonnenberg-signal
Copy link
Contributor

Thanks for your work on this. We've merged it in our internal branches, and it will be in 6.23.x.

@josh-signal josh-signal merged commit ebde2a1 into signalapp:main Jun 21, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants