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

use animint title in pages README #147

Merged
merged 5 commits into from
Aug 30, 2024
Merged

use animint title in pages README #147

merged 5 commits into from
Aug 30, 2024

Conversation

tdhock
Copy link
Collaborator

@tdhock tdhock commented Aug 29, 2024

closes #146

@tdhock
Copy link
Collaborator Author

tdhock commented Aug 29, 2024

hi @Faye-yufan can you please review this PR?

I believe it solves the issues you ran into in #138 (no permission to delete/create test repo).

I created a new org, animint-test which should contain only repos relevant to testing animint2. This is different from animint org which contains the animint2 repo / project source code.

I created a new fine-grained personal access token which has permission to create/delete any repos under animint-test org. Since there is no important data in animint-test org, there is no risk even if a malicious user gets control of this token. (animint2 source code is under animint org, where the token does not have permission)

I updated test-compiler-ghpages.R with documentation of the new process, and additional tests. First the old test repo is deleted, then a new one is created, then we run animint2pages, twice, to test results of creation and update.

Does that make sense? Please review, and tell me if there is anything you think I could do to improve the code or docs.

Copy link
Contributor

@Faye-yufan Faye-yufan left a comment

Choose a reason for hiding this comment

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

@tdhock Thank you for creating the detailed test for animint test repo. So glad that it passed on GH actions. But I have not tried the test on my local. Did you try run on your local?

R/z_pages.R Show resolved Hide resolved
## - Repository access: All repositories
## - Repository permissions: Administration and Contents: read and write.
## - Generate token
## - copy token and paste into PAT_GITHUB on https://github.com/animint/animint2/settings/secrets/actions
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that it has already been set, so I don't need to set the PAT again, correct?
Will the PAT expire?

Copy link
Collaborator Author

@tdhock tdhock Aug 29, 2024

Choose a reason for hiding this comment

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

I set the PAT to expire in 1 year (that is the max on github)
If you try to run the test locally, it should work, as long as your github token has the right permissions. (it worked for me locally when I used the same token as is currently used on github actions)
Since I added you to the animint-test org as member (can create new repositories), you should be able to run the test code locally with your regular github credentials (no special token required).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually I just tried that myself and I got

> gh::gh("DELETE /repos/animint-test/animint2pages_test_repo")
Error in `gh::gh()`:
! GitHub API error (403): Must have admin rights to Repository.Read more at <https://docs.github.com/rest/repos/repos#delete-a-repository>
Run `rlang::last_trace()` to see where the error occurred.

but I am owner of animint-test org???

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to change base permissions of animint-test org from Read to Admin
image
https://github.com/organizations/animint-test/settings/member_privileges?enable_tip=#base-permissions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so I got it to work for local testing by doing the following.
first I used gitcreds::gitcreds_delete() to remove my previous git crendential/token, which I believe only has repo read/write permission (not delete).
then I go to https://github.com/settings/tokens/new and check repo and delete_repo then generate token, as shown below.
image
Then I open up git bash on windows, type git push, then a window pops up, click token, paste the token, confirm.
Then running the tests locally should work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some LOCAL TESTING docs in the comments next to this test, does that make sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thank you! I will try on my local tonight.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested it locally and got it to work!
It seems gitcreds::gitcreds_delete() wasn't deleting my old token, so I used credentials::set_github_pat("my-pat") to reset it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great thanks, I moved the docs to the wiki https://github.com/animint/animint2/wiki/Testing#installing-a-github-token-for-use-locally and I mentioned your credentials::set_github_pat("my-pat") trick there.
hopefully this technique will make it easier to implement tests in #138

Toby Dylan Hocking and others added 2 commits August 29, 2024 16:18
@tdhock tdhock merged commit 994006d into master Aug 30, 2024
3 checks passed
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.

animint2pages README title could be more informative
2 participants