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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix use_github for https protocol with github PAT #340

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@cderv
Contributor

cderv commented Apr 29, 2018

This PR fixes #320 (use_github does not push with https protocol and GITHUB_PAT), closes #334 and contributes to #322.

As discussed, if no token is provided as argument (auth_token = NULL) in use_github, the default expected behaviour (GITHUB_PAT or GITHUB_TOKEN env var) is used through gh_token().

For use_github to work, I needed to deal with git2r cran or dev version. This is now included:

  • use git2r::head for version <= 0.21.0 (currently cran version of git2r)
  • use new git2r::repository_head for version > 0.21.0 (currently dev version of git2r

Lastly, I included a manual test file for use_github and https. I need to set up ssh on my computer to add ssh test. Could do it but in another PR.
I think manual test file are great for this kind of interactive 馃摝 like usethis. However, I am thinking that it could be improved by a Rmd test report or using testthat in manual test script. If your are insterested, I am willing to work on something.

Hope this PR suits you. Tell me what I need to adjust if necessary.

cderv added some commits Apr 3, 2018

get the PAT is auth_token is NULL with git2r
git2r does not have the same way to handle token than gh. A non-null token must be provided so we need to retrieve the token if auth_token is null.
@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc May 24, 2018

Member

I merged this manually and added a few more commits. But yours are there and I added a NEWS bullet. Thanks!

Member

jennybc commented May 24, 2018

I merged this manually and added a few more commits. But yours are there and I added a NEWS bullet. Thanks!

@jennybc jennybc closed this May 24, 2018

@cderv

This comment has been minimized.

Show comment
Hide comment
@cderv

cderv May 24, 2018

Contributor

Perfect thanks.

I see you have simplified the manual test - I'll keep this good practice on mind for next time.

Glad I could help and contribute!

See you in the next issue or PR! 馃槈

Contributor

cderv commented May 24, 2018

Perfect thanks.

I see you have simplified the manual test - I'll keep this good practice on mind for next time.

Glad I could help and contribute!

See you in the next issue or PR! 馃槈

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc May 24, 2018

Member

I see you have simplified the manual test - I'll keep this good practice on mind for next time.

Well, we don't really have standards for these. But I did decide to make it a bit more light weight, more so that anyone who visits doesn't need to read as much code. It makes things less automatic, no doubt, but I figure that is OK for a manual test.

Member

jennybc commented May 24, 2018

I see you have simplified the manual test - I'll keep this good practice on mind for next time.

Well, we don't really have standards for these. But I did decide to make it a bit more light weight, more so that anyone who visits doesn't need to read as much code. It makes things less automatic, no doubt, but I figure that is OK for a manual test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment