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

Add Support for GitHub Personal Access Tokens #18

Merged
merged 13 commits into from
Sep 6, 2022

Conversation

sasial-dev
Copy link
Contributor

@sasial-dev sasial-dev commented Jun 19, 2022

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:

  • A new auth file has been created (auth.toml) inside ~/.aftman
  • GitHubSource now pulls auth from the file ^

Out of scope for this PR are:

  • CLI arguments to add/control the auth manifest file. Currently only manual editing will be suupported.

* 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 tools It has been confirmed users do hit the ratelimit.

@sasial-dev
Copy link
Contributor Author

The biggest thing is reading the manifest to check for the token before using aftman add/aftman run. I wasn't sure how/where you wanted me to read the manifest - am looking for your direction here before so I don't do it in the wrong place

Copy link
Owner

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

Minor feedback -- let's think about how we should store authentication tokens and what the authentication workflow should look like.

src/manifest.rs Outdated Show resolved Hide resolved
src/cli/mod.rs Outdated Show resolved Hide resolved
@sasial-dev
Copy link
Contributor Author

Code-wise, this looks much better!

@sasial-dev sasial-dev requested a review from LPGhatguy July 3, 2022 06:47
@sasial-dev
Copy link
Contributor Author

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)

@LPGhatguy
Copy link
Owner

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.

src/auth.rs Show resolved Hide resolved
src/auth.rs Show resolved Hide resolved
src/tool_source/github.rs Show resolved Hide resolved
@ok-nick
Copy link

ok-nick commented Jul 18, 2022

This PR is definitely useful, I've hit the 60 rate limit via CI.

@filiptibell
Copy link
Contributor

filiptibell commented Aug 9, 2022

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 🎉

Copy link
Owner

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

Thanks for everyone's patience on this! This should be the final review before this is good to go.

src/tool_source/github.rs Show resolved Hide resolved
}

impl GitHubSource {
pub fn new() -> Self {
pub fn new(home: &Home) -> Self {
let token = AuthManifest::load(home).ok();
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've attempted to fix this, but will need some help with borrowing as I'm still new to rust!

src/tool_source/github.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
src/auth.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

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>) -> Self

and 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()));

src/tool_source/github.rs Outdated Show resolved Hide resolved
src/tool_source/github.rs Outdated Show resolved Hide resolved
src/tool_storage.rs Outdated Show resolved Hide resolved
src/tool_storage.rs Outdated Show resolved Hide resolved
Co-authored-by: Filip Tibell <filip.tibell@gmail.com>
@sasial-dev
Copy link
Contributor Author

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 :)

@sasial-dev sasial-dev changed the title Add support for personal access tokens Add Support for GitHub Personal Access Tokens Sep 5, 2022
Copy link
Owner

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

Looks great! Thank for filing this PR and for everyone's patience.

@LPGhatguy LPGhatguy merged commit 946ffba into LPGhatguy:main Sep 6, 2022
@sasial-dev sasial-dev deleted the personal-access-token branch September 6, 2022 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auth for private repositories
4 participants