Add Support for GitHub Personal Access Tokens#18
Conversation
|
The biggest thing is reading the manifest to check for the token before using |
LPGhatguy
left a comment
There was a problem hiding this comment.
Minor feedback -- let's think about how we should store authentication tokens and what the authentication workflow should look like.
|
Code-wise, this looks much better! |
884f848 to
ccbbc51
Compare
|
Note that the CI failed due to warning: add_token not being implmented yet, let me know what course of action you'd like me to take (leaving it as deadcode or implmenting a CLI command) |
CI failed because you didn't run rustfmt! You should turn on "format on save" in your editor so that code gets correctly formatted. |
|
This PR is definitely useful, I've hit the 60 rate limit via CI. |
|
I can second how useful this would be for us, right now we're unable to completely manage our tools in CI using neither foreman nor aftman. Foreman does not match release artifacts properly for some third party tools we need (such as sentry-cli) and has issues with long-running processes. Aftman is only missing support for private tools/auth. We could completely get rid of our self-hosted and custom setup runners whenever this PR goes through 🎉 |
LPGhatguy
left a comment
There was a problem hiding this comment.
Thanks for everyone's patience on this! This should be the final review before this is good to go.
| impl GitHubSource { | ||
| pub fn new() -> Self { | ||
| pub fn new(home: &Home) -> Self { | ||
| let token = AuthManifest::load(home).ok(); |
There was a problem hiding this comment.
Should we load this file in another place? We shouldn't be suppressing errors here with ok(). If we want to load it here, we can change GitHubSource::new to return an anyhow::Result<Self> instead.
There was a problem hiding this comment.
I've attempted to fix this, but will need some help with borrowing as I'm still new to rust!
Co-authored-by: Lucien Greathouse <me@lpghatguy.com>
Co-authored-by: Lucien Greathouse <me@lpghatguy.com>
a415670 to
3437ec6
Compare
There was a problem hiding this comment.
Some feedback on how you could solve the issues you were having related to ownership.
I feel like GithubSource might not need to see the entire auth manifest, just what is relevant to itself. That does make the solution below look a little bit clunky, so if you do still want GithubSource::new to take an AuthManifest you should have it take a reference instead
pub fn new(auth: Option<&AuthManifest>) -> Selfand clone the personal access token in there:
token: auth.and_then(|t| t.github.clone())Then, to construct:
let github = self
.github
.get_or_init(|| GitHubSource::new(self.auth.as_ref()));76e0838 to
b9f6990
Compare
Co-authored-by: Filip Tibell <filip.tibell@gmail.com>
b9f6990 to
c106675
Compare
|
Thanks again @filiptibell! I actually tried and pushed commits implimenting both options here! I finally decided on passing the whole manifest for future compatibility (some sources may need more than 1 field etc) and it felt like a cleaner soulution :) |
LPGhatguy
left a comment
There was a problem hiding this comment.
Looks great! Thank for filing this PR and for everyone's patience.
Fixes #7
This is my first time using rust, so please let me know if anything needs to be changed as i'm really new to the language!
This PR consists of:
auth.toml) inside~/.aftmanOut of scope for this PR are:
* While (hopfully) no one will hit that rate limit, it's nice to have to option of so many more requests. The first usecase I could think of was for a new user downloading releases for lots of toolsIt has been confirmed users do hit the ratelimit.