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.

Sign up for GitHub

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

Jump to bottom

Support private github repository #1690

Merged
merged 58 commits into from
Jun 10, 2024
Merged

Support private github repository #1690

merged 58 commits into from
Jun 10, 2024

Conversation

NobodyXu
Copy link
Member

@NobodyXu NobodyXu commented May 5, 2024

Fixed #1681

@NobodyXu
Refactor: Create new crate binstalk-git-repo-api
9efe395 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix CI lint warnings
90670a6 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix just check: Rm deleted features from cargo-hack check
8bda7ea 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Refactor: Extract new mod error
d2914cc 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Ret artifact url in has_release_artifact
0708f6f 
So that we can use it to download from private repositories.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Move test_graph_ql_error_type to mod error
30a4a53 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix running cargo test in binstalk-git-repo-api`
e06ea36 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Remove unnecessary import in mod error::test
645a2f2 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Rename mod request to release_artifacts`
6c67c7d 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Impl draft version of fetching repo info
5417475 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Refactor: Move HasReleaseArtifacts failure variants into GhApiError
7bd8468 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Refactor: Use GhRepo in GhRelease
6e6d7d7 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix testing
f9008d7 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@scullionw
Copy link

is there anything we can do to help move this forward? maybe testing a build?

@NobodyXu
Copy link
Member Author

is there anything we can do to help move this forward? maybe testing a build?

I just need to spend more time on this, I'm a bit busy recently, but rest asure, I didn't forget it.

@NobodyXu
Refactor: Return 'static future
f90eea4 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Member Author

Update: I'm a bit busy this week.

cc @scullionw we actually need more contributors for cargo-binstall.

If you are willing to submit a PR, then I can do a code review very quickly.

If you have time, you could pick up where I have left and I'm willing to answer any questions you have related to the codebase.

@NobodyXu
Merge branch 'main' into feat/private-github-repo
c36145f
@NobodyXu
Refactor: Make sure 'static Future is returned
cb418fd 
To make it easier to create generic function

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Add logging to unit testing
fd5b78c 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix unit testing
361aca4 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Refactor: Extract new fn GhApiClient::do_fetch
1350ac3 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Rm unused percent_encode_http_url_path
ffe47d3 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix cargo test run on CI
962a7d0 
`cargo test` run all tests in one process.

As such, `set_global_default` would fail on the second call.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Optimize GhApiClient::do_fetch: Avoid unnecessary restful API call
fc02228 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Refactor: Rm param auth_token for restful API fn
f824ebb 
which is always set to `None`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix get_repo_info: Retry on rate limit
bc71a1b 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Improve debug! logging
a295fe9 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Add more debug logging
e02321e 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Add more debugging
23a1bed 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Add more debug logging
a236f53 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Member Author

NobodyXu commented Jun 9, 2024

When debugging CI failure, I've found:

DEBUG has_release_artifact{release=GhRelease { repo: GhRepo { owner: "cargo-bins", repo: "private-repo-for-testing" }, tag: "0.12.0" } artifact_name="cargo-binstall-x86_64-unknown-linux-musl-v0.12.0.tar"}: response = GraphQLResponse { data: GraphQLData { repository: Some(GraphQLRepo { release: None }) }, errors: None }

seems that somehow release artifact GraphQL cannot found the release in the repository, despite it being present in the repo.

@NobodyXu
Copy link
Member Author

NobodyXu commented Jun 9, 2024

Ok turns out that I forgot to give the token enough permission.

I've recreated github token, using another machine account which only has read-only access to the repository, and give the new token full access to the private repostories of that machine account, since it has read-only access anyway.

NobodyXu
NobodyXu commented Jun 9, 2024
View reviewed changes
e2e-tests/private-github-repo.sh Outdated Show resolved Hide resolved
crates/binstalk-git-repo-api/src/gh_api_client/release_artifacts.rs Outdated Show resolved Hide resolved
crates/binstalk-git-repo-api/src/gh_api_client.rs Outdated Show resolved Hide resolved
crates/binstalk-git-repo-api/src/gh_api_client.rs Outdated Show resolved Hide resolved
@NobodyXu
Apply suggestions from code review
d118412
@NobodyXu
Fix compilation
25b216c 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Fix cargo fmt
2b19b9a 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu marked this pull request as ready for review June 9, 2024 14:48
@NobodyXu NobodyXu requested a review from passcod June 9, 2024 14:59
@NobodyXu
Copy link
Member Author

cc @scullionw This PR is now ready for review, you should be able to download from private repositories using this PR, could you have a try please?

@NobodyXu NobodyXu added this pull request to the merge queue Jun 10, 2024
@NobodyXu NobodyXu removed this pull request from the merge queue due to a manual request Jun 10, 2024
NobodyXu
NobodyXu commented Jun 10, 2024
View reviewed changes
Copy link
Member Author

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

It seems that we have hit the gh token limit for CI, since many tests are run under it.

will apply the suggestion and re-merge it later.

crates/binstalk-git-repo-api/Cargo.toml Outdated Show resolved Hide resolved
NobodyXu
NobodyXu commented Jun 10, 2024
View reviewed changes
crates/binstalk-fetchers/Cargo.toml Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
NobodyXu
NobodyXu commented Jun 10, 2024
View reviewed changes
crates/binstalk/Cargo.toml Outdated Show resolved Hide resolved
@NobodyXu NobodyXu added this pull request to the merge queue Jun 10, 2024
Merged via the queue into main with commit 1dbd246 Jun 10, 2024
28 checks passed
@NobodyXu NobodyXu deleted the feat/private-github-repo branch June 10, 2024 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@passcod passcod passcod approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: cannot access private repositor with GITHUB_TOKEN provided
3 participants
@NobodyXu @scullionw @passcod