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

feat: Add package for pull request and comment #2693

Merged
merged 4 commits into from
Jun 12, 2023

Conversation

yanguoyu
Copy link
Collaborator

@yanguoyu yanguoyu commented May 31, 2023

Because the trigger on workflow_run needs the action's yml file to exist on the default branch.
yanguoyu#38 (comment) is an example of a comment package for committing to PR.

  1. Add pull_request trigger for the Package Neuron for Test action
  2. Skip the code sign if there not exist certificate
  3. Because for pull_request action, there is no access to write comments. Add a workflow to add comments on the PR. https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#restrictions-on-repository-forks. Then when the trigger on push event, add a comment on the commit, else add a comment on the PR.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Jun 1, 2023

Because the trigger on workflow_run needs the action's yml file to exist on the default branch. yanguoyu#38 (comment) is an example of a comment package for committing to PR.

  1. Add pull_request trigger for the Package Neuron for Test action
  2. When the action is triggered by pull_request event, packaging without code sign.
  3. After package success, comment on the PR with the package result.

It requires an extra action to sign a package, what if sign packages when the PR author is permitted, as we did in add-replied-label(https://github.com/nervosnetwork/neuron/blob/develop/.github/workflows/add-replied-label.yml#L13-L29)

@Keith-CY
Copy link
Collaborator

Keith-CY commented Jun 1, 2023

Because the trigger on workflow_run needs the action's yml file to exist on the default branch. yanguoyu#38 (comment) is an example of a comment package for committing to PR.

  1. Add pull_request trigger for the Package Neuron for Test action
  2. When the action is triggered by pull_request event, packaging without code sign.
  3. After package success, comment on the PR with the package result.

It requires an extra action to sign a package, what if sign packages when the PR author is permitted, as we did in add-replied-label(develop/.github/workflows/add-replied-label.yml#L13-L29)

The original intention is to skip code sign when certificates are missing. So sign-when-permitted is not good enough too because code sign will be skipped even the contributor has his/her own certificates.

Ref: Magickbase/neuron-public-issues#154

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Jun 1, 2023

Because the trigger on workflow_run needs the action's yml file to exist on the default branch. yanguoyu#38 (comment) is an example of a comment package for committing to PR.

  1. Add pull_request trigger for the Package Neuron for Test action
  2. When the action is triggered by pull_request event, packaging without code sign.
  3. After package success, comment on the PR with the package result.

It requires an extra action to sign a package, what if sign packages when the PR author is permitted, as we did in add-replied-label(https://github.com/nervosnetwork/neuron/blob/develop/.github/workflows/add-replied-label.yml#L13-L29)

Workflows in forked repositories
Workflows don't run in forked repositories by default. You must enable GitHub Actions in the Actions tab of the forked repository.
With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository. The GITHUB_TOKEN has read-only permissions in pull requests from forked repositories. For more information, see "Automatic token authentication."

From the GitHub action document, because most of the PR is opened by the forked repository, the action cannot read other secrets. But for every push event, it will sign with certificates. it's enough to sign on push event.
Then every package in the PR comment will be unsigned, and every package in the commit message will be signed.

Developers can get the signed package when they push codes to their repository if they have certificates.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Jun 1, 2023

every package in the PR comment will be unsigned, and every package in the commit message will be signed.

If I forked Neuron to Keith-CY/neuron and I don't have certificates configured in Keith-CY/neuron, will packages of my commits be signed?


If sign code depends on if there's a certificate, I can fork nervosnetwork/neuron to Keith-CY/neuron, and sign every package of a commit. There's no need to have an unsigned neuron if I have a certificate in Keith-CY/neuron.

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Jun 1, 2023

If I forked Neuron to Keith-CY/neuron and I don't have certificates configured in Keith-CY/neuron, will my commits be signed?

It will try to sign but will fail.

I want to keep uniform rules, then every pr comment package will be unsigned, and if a commit comment exists it will be a signed package.
For skip code sign when certificates are missing. Then Sometimes the package is signed, and sometimes the package is unsigned.

For the original intention, I will change the GitHub action, to skip code sign when certificates are missing.

@yanguoyu yanguoyu marked this pull request as draft June 1, 2023 08:51
@yanguoyu yanguoyu force-pushed the test-pr-package branch 4 times, most recently from eacdc13 to 184bd70 Compare June 1, 2023 12:57
@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Jun 1, 2023

  1. Windows code sign and skip Mac code sign.
    yanguoyu@fe2ecd6#comments is an example of a commit comment
    test: example yanguoyu/neuron#39 (comment) is an example for PR comment.

  2. Mac code sign and skip window code sign
    https://github.com/yanguoyu/neuron/actions/runs/5145041996?pr=39 run failed caused of the wrong apple id.

@yanguoyu yanguoyu marked this pull request as ready for review June 1, 2023 13:44
@yanguoyu yanguoyu force-pushed the test-pr-package branch 2 times, most recently from 80fb9e4 to a162a29 Compare June 2, 2023 00:38
@Keith-CY
Copy link
Collaborator

Keith-CY commented Jun 2, 2023

@yanguoyu yanguoyu force-pushed the test-pr-package branch 2 times, most recently from 1e2f1d7 to 6e9783c Compare June 2, 2023 06:08
packages/neuron-wallet/scripts/notarize.js Outdated Show resolved Hide resolved
.github/workflows/package_for_test.yml Outdated Show resolved Hide resolved
.github/workflows/package_for_test.yml Show resolved Hide resolved
@yanguoyu yanguoyu requested a review from WhiteMinds June 11, 2023 07:44
@Keith-CY Keith-CY merged commit e7adec4 into nervosnetwork:develop Jun 12, 2023
@yanguoyu yanguoyu deleted the test-pr-package branch June 13, 2023 02:24
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.

4 participants