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

With github pullrequests work on target repo rather than the source #816

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timor-raiman
Copy link

This provides support for private forks, where GITHUB_TOKEN generated by GH in the target repo pull request context does not provide access to the pull request's source repo.

This requires vcstool to be fixed to properly support checking out sources by hash, rather than a branch or tag, because a pull request's source branch does not exist in the destination repository. Rather, the patch can be accessed as refs/pr/123/head or refs/pr/123/merge. With github.context.sha being equivalent to the resolved commit hash of refs/pr/123/merge.

@timor-raiman timor-raiman requested a review from a team as a code owner April 20, 2023 11:22
@timor-raiman timor-raiman requested review from christophebedard and jhdcs and removed request for a team April 20, 2023 11:22
Signed-off-by: Timor Raiman <timor.raiman@intel.com>
@Nir-Az
Copy link

Nir-Az commented Apr 23, 2023

Any change someone can take a look on this 2 PR's?
It should fix the issues we see.
Otherwise we cannot use this tool as our builds are failing..
Thanks

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.34 ⚠️

Comparison is base (fc52906) 47.87% compared to head (907af1e) 47.53%.

❗ Current head 907af1e differs from pull request most recent head d18eee4. Consider uploading reports for the commit d18eee4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
- Coverage   47.87%   47.53%   -0.34%     
==========================================
  Files           2        2              
  Lines         282      284       +2     
  Branches       76       77       +1     
==========================================
  Hits          135      135              
- Misses        147      149       +2     
Impacted Files Coverage Δ
src/action-ros-ci.ts 41.56% <0.00%> (-0.33%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@christophebedard
Copy link
Member

The problem is that there is currently no active maintainer for vcstool. This will not get fixed without someone submitting a PR to the vcstool repo with a fix, but even then there is no active maintainer to merge it. I know people are trying to figure this out, but it might take a bit of time.

We could consider using a custom/forked version of vcstool with the fix needed for this in action-ros-ci itself, but then again someone needs to submit a fix.

@@ -555,10 +555,13 @@ done`;
// being built.
let repoFullName = process.env.GITHUB_REPOSITORY as string;
if (github.context.payload.pull_request) {
repoFullName = github.context.payload.pull_request.head.repo.full_name;
repoFullName = github.context.payload.pull_request.base.repo.full_name;
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the git commit for the "PR merge" commit that we are trying to check out is only available on the target repo (i.e., the original repo against which the PR was opened), correct?

Copy link
Author

Choose a reason for hiding this comment

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

correct. But even more critically, the token which GH generates automatically in the target repo is not valid in the source repo (which is not public).

Copy link

Choose a reason for hiding this comment

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

@christophebedard since there is no one maintaining vcstool, and this tool is maintained.
Would you consider dropping usage of vctools and clone repos in a different way?
Might be easier then trying to fix it in 2 repos

Copy link
Member

Choose a reason for hiding this comment

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

But even more critically, the token which GH generates automatically in the target repo is not valid in the source repo (which is not public).

Got it.

since there is no one maintaining vcstool, and this tool is maintained.

The goal is for OSRF to get maintainer access eventually, but that's going to take some time.

Would you consider dropping usage of vctools and clone repos in a different way?
Might be easier then trying to fix it in 2 repos

For the repo being tested, yes, that's a good idea. Running plain old git commands instead of using vcstool would just be simpler here. Would you be willing to make this change? I wouldn't change anything about the vcs-repo-file-url option (for other repos), though.

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.

None yet

3 participants