-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat(config): add option to set token from file #171
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the implementation! I left some comments.
140f3e6
to
a53dc83
Compare
6143b18
to
2512fac
Compare
config.toml
Outdated
# Token used for creating and accessing files. | ||
auth_token = "" | ||
# Leading and trailing whitespace is trimmed, overrides delete_token if set. | ||
auth_token_file = "~/example/auth-token" | ||
# Token used for deleting files. | ||
delete_token = "" | ||
# Leading and trailing whitespace is trimmed, overrides delete_token if set. | ||
delete_token_file = "~/example/delete-token" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Token used for creating and accessing files. | |
auth_token = "" | |
# Leading and trailing whitespace is trimmed, overrides delete_token if set. | |
auth_token_file = "~/example/auth-token" | |
# Token used for deleting files. | |
delete_token = "" | |
# Leading and trailing whitespace is trimmed, overrides delete_token if set. | |
delete_token_file = "~/example/delete-token" | |
# Server authentication token | |
auth_token = "" | |
# A file that contains an authentication token | |
auth_token_file = "~/example/auth-token" | |
# Server deletion token | |
delete_token = "" | |
# A file that contains a deletion token | |
delete_token_file = "~/example/delete-token" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a minor change ^^^
an deletion -> a deletion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nydragon you have resolved both of orhun's suggestions (which he added 2 weeks ago), but not made these changes to your code.
Are you still working on this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a bit verbose and the info is already in the documentation. It's not necessary to repeat it in the config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove all the 4 comments then ? Or just keep it concise ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with leaving them, and maybe making them a bit concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Orhun suggested how to change them back then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must be blind, thanks for the patience! Pushed the changes
ping @tessus for review |
Will review this evening. Getting ready for an appointment right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In one of the original comments it states:
overrides auth/delete_token if set.
I just want to make sure that even if the file is set in the config file, the token can be overridden via the command line argument -a
.
Additionally, maybe the fact that whitespace characters are trimmed should be mentioned in the README. Actually the README needs an update with those 2 options anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the README.md updates, I'd suggest applying this patch (I can't push to your branch because I don't have access):
-or specify them in the [configuration file](#configuration).
+or specify them in the [configuration file](#configuration) as follows:
+
+```toml
+[server]
+auth_token = "<token>"
+delete_token = "<delete_token>"
+```
+
+It is also possible to use these tokens from a file:
+
+```toml
+[server]
+auth_token_file = "~/example/auth-token"
+delete_token_file = "~/example/delete-token"
+```
+
+The contents should be only the token, the newlines will be trimmed.
@orhun the OP hasn't commited the changes you suggested 2 weeks ago and since then it's been silent in this PR. Not sure what is going on. I mentioned him in another comment. Let's see if there's a reply. |
We can probably create a new PR from these changes and then merge afterwards if things are silent :) |
Hey, sorry for the wait, I applied the requested changes to the branch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I left a message inline. Other than that, LGTM! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Add the option to pass token to rustypaste-cli from files, this is useful in scenarios where one manages their secrets with agenix (and similar workflows).
New dependencies:
closes #170