Change the directory certificates are stored in #28
Conversation
|
@ioquatix I have configured my editor to automatically strip trailing whitespace, so I've gone and applied that to every ruby file in the project (and updating editorconfig to match). If you don't want that, I can easily revert the change. And if you didn't know, github has a feature to hide whitespace changes in the diff view. Makes it easier to see the actual changes outside of the formatting change. 😄 |
|
I don't normally strip trailing whitespace, and would prefer if that change wasn't included in this PR thanks :) |
The new directory is compliant with the XDG Basedir specification[1]. The library will migrate the old directory to the new directory when it is not destructive. [1]: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
|
Rebased to remove the misc whitespace and CI iteration commits. 😄 |
Make sure to require FileUtils Co-authored-by: Samuel Williams <samuel.williams@oriontransfer.co.nz>
lib/localhost/authority.rb
Outdated
| @@ -196,6 +197,7 @@ def load(path = @root) | |||
| end | |||
|
|
|||
| def save(path = @root) | |||
| migrate_legacy_authority_path(path) | |||
| Dir.mkdir(path, 0700) unless File.directory?(path) | |||
There was a problem hiding this comment.
This line fails on external tests, where the XDG state directory doesn't exist.
I wonder if we should use FileUtils.mkdir_p here instead?
Should we be creating .local if it doesn't exist?
There was a problem hiding this comment.
See https://github.com/socketry/localhost/actions/runs/8656328284/job/23736689683?pr=28#step:4:248 for the actual failure.
There was a problem hiding this comment.
The question is, for users who don't use .local, is it appropriate for random applications to create it? Does XDG have guidelines we can follow?
There was a problem hiding this comment.
Yeah, I was thinking about that. It seems like we should. The basedir spec doesn't seem to have a strong opinion.
There was a problem hiding this comment.
Okay, what I'd suggest we do, is move that into a separate operation, at least initially.
There was a problem hiding this comment.
I think initially, let's detect if we need to do it in Authority.path.
There was a problem hiding this comment.
If, when attempting to write a file, the destination directory is non-existent an attempt should be made to create it with permission 0700. If the destination directory exists already the permissions should not be changed. The application should be prepared to handle the case where the file could not be written, either because the directory was non-existent and could not be created, or for any other reason. In such case it may choose to present an error message to the user.
is what the spec says, immediately after discussing the config directory, but I think it applies to stateful files like this as well.
For non-Linux systems, like MacOS, XDG doesn't make sense and yet it also makes sense. (It seems like CLI tools will use XDG anyways, but graphical ones will use ~/Library/)
|
@ioquatix since we're considering doing I/O actions in Authority.path, should the migration be merged into that method as well? I had split it out into a separate method in order to avoid any side-effects from instantiation, but that would no longer be true with the call to FileUtils. |
Yes, I think that's a good idea. |
|
I think the three failing checks aren't related, so this is finally ready for merging if you want. 😸 |
|
Thanks, this looks great, I'll test it locally and cut a release. |
The new directory is compliant with the XDG Basedir specification.
The library will migrate the old directory to the new directory when it is not destructive.
Closes #27 .
Types of Changes
Contribution