-
Notifications
You must be signed in to change notification settings - Fork 19
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
use gitcreds for managing github PAT #96
base: master
Are you sure you want to change the base?
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.
Nice work. In this instance, the CI/CD checks failed, which I'm assuming is due to them needing to be updated. I will take a look at them and see if I can get them working again.
I figured out that the reason the checks failed is because you didn't have the correct OAuth token for the CI/CD workflows. I'll see if I can generate one and run the tests myself. |
I'm having trouble creating an OAuth token for the CI/CD workflows, and I don't want to force push to the main branch. Please let me know your availability for the rest of today and the rest of this week. |
I can also take a look at this while I finish up this PR. I would think that the built-in PAT that GitHub actions uses would have gist scope. Please do wait to merge this until it is no longer a "draft" and we get the checks passing. |
Yeah, looks like the built-in |
…x CI/CD errors.
I just tried to add it, and I updated the yaml file at |
…EN to fix CI/CD errors." This reverts commit f0bf8ef.
So I thought you could add the "gist" scope with the I take it the |
…GIST_TOKEN to fix CI/CD errors."" This reverts commit ca9c281.
@Aariq The checks are failing because the current tokens do not have gist management permissions. I tried creating my own fine-grained PAT with the appropriate gist scope and installing it as a secret, but I think someone from rOpenSci is going to have to create an organizational token for us. I also tried to implement my code changes on top of yours in the hopes of simplifying things. However, without the PAT with the correct permissions, it's a moot point. Thus, feel free to roll back my changes or let me know if you'd like for me to do so. |
Huh, I think I'm either misunderstanding something about
So either |
test_that("PAT gets found", {
expect_equal(gitcreds::gitcreds_get()$password, Sys.getenv("GITHUB_PAT"))
expect_equal(gistr::gist_auth()$Authorization, paste0("token ", Sys.getenv("GITHUB_PAT")))
}) I was able to get the above simple tests passing in a dummy package by doing the following:
I think the same should work here. |
It seems we may be addressing two separate issues here, so I’ll elaborate on each below. Issue 1: Testing
|
num | time | notes |
---|---|---|
1 | 2024-11-07 11:16 GMT-6 | Posting the corrected version of my response. For the previous version, I inadvertently posted my first draft. The version is the one I meant to post. |
I think you could just use a PAT you create for your own account to get the tests to pass, but I understand why you wouldn't want to do that—it apparently fills up your gists with a bunch of artifacts from the tests running. |
Why not mock the tests instead? Not that's it's super easy either, but it might solve the problem of having tests that create and delete gists. https://books.ropensci.org/http-testing/ |
Regarding secrets, if a PAT is created for this repo, it will only be accessible for workflow runs from this repository, that is to say, not from external PRs. |
DESCRIPTION
Outdated
person("Ramnath", "Vaidyanathan", role = "aut"), | ||
person("Karthik", "Ram", role = "aut"), | ||
person("Milgram", "Eric", role = "aut") | ||
person("Milgram", "Eric", role = "aut"), |
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.
it should be both aut and cre, "cre" would then be removed from Scott's roles
gitcreds::gitcreds_get() | ||
) | ||
token <- creds$password | ||
#TODO would be great to check here that token has "gist" scope |
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 <- auth$credentials$access_token | ||
} else { | ||
stop("In non-interactive environments, please set GITHUB_PAT env to a GitHub", | ||
" access token (https://help.github.com/articles/creating-an-access-token-for-command-line-use)", |
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 could also point to https://usethis.r-lib.org/reference/github-token.html as the functions are quite handy
@@ -56,3 +72,11 @@ gistr_app <- httr::oauth_app( | |||
"89ecf04527f70e0f9730", | |||
"77b5970cdeda925513b2cdec40c309ea384b74b7" | |||
) | |||
|
|||
# inspired by https://github.com/r-lib/gh/blob/main/R/gh_token.R | |||
valid_gh_pat <- function(x) { |
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.
instead, why not directly test it works, using a call to https://docs.github.com/en/rest/users/users?apiVersion=2022-11-28#get-the-authenticated-user
Co-authored-by: Maëlle Salmon <[email protected]>
Description
Still a WIP, but this PR aims to re-write the
gist_auth()
function to take advantage ofgitcreds::gitcreds_get()
which finds github PATs stored either in theGITHUB_PAT
env var or stored withgitcreds::gitcreds_set()
Changes:
Would be nice:
gist_auth()
check that the PAT has gist scopeRelated Issue
closes #95
Example