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

Ability to request with a certain scope #2

Merged
merged 7 commits into from
Jan 26, 2024

Conversation

rhaskia
Copy link
Contributor

@rhaskia rhaskia commented Jan 2, 2024

No description provided.

Copy link
Owner

@jakewilkins jakewilkins 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 the PR!

I use this with GitHub Apps so passing scopes never crossed my mind 🤦‍♂️

I'd like to merge this but could use a little more detail on some of the changes.

Cargo.toml Outdated
@@ -10,10 +10,9 @@ repository = "https://github.com/jakewilkins/gh-device-flow"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
reqwest = { version = "0.11", features = ["blocking", "json"] }
reqwest = { version = "0.11", features = ["blocking", "json", "rustls-tls"] }
Copy link
Owner

Choose a reason for hiding this comment

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

Can you provide some more context here on why this change is required and what sort of validation you've done that it is non-breaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Openssl-sys was taking a long time to compile on my end, so I found it could be replaced with rust-ls which seems to run better. Reqwest hasn't thrown any errors in the time that I've switched so I don't think there is much difference apart from performance.

Copy link
Owner

Choose a reason for hiding this comment

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

rustls-tls fails to compile correctly on the arm or armv7 builds.

If you know of a simple way to make those work I'm glad to hear it, or if you're okay with merging without the rustls-tls change I'm happy to have that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it still runs as normal without it that is good with me.

Copy link
Owner

Choose a reason for hiding this comment

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

I know that there is a way for me to make this work that would allow you to choose the one that's best for you but I haven't ever done that before and don't have the time to mess with figuring it right now 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Owner

Choose a reason for hiding this comment

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

What I had in mind was something similar to #3 but I just kind of feel like I'm misusing Cargo in some way and if I could find the right options things would go better.

@jakewilkins jakewilkins merged commit a2298cf into jakewilkins:main Jan 26, 2024
0 of 5 checks passed
@rhaskia
Copy link
Contributor Author

rhaskia commented Mar 15, 2024

Oh, I wanted to upload an app which has this as a dependency to crates.io but I cannot as it seems the crates.io version of this does not have the scope feature yet. Would you be able to update the crates.io at some point when you are free?

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.

2 participants