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][CI] Add approval solution to reduce GitHub Actions resource consumption #17693

Merged
merged 9 commits into from
Sep 19, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Sep 16, 2022

Motivation

Mailing list discussion: https://lists.apache.org/thread/gwfmxmxlhtsjn17sxxc367jcs4pcwofz

To reduce resource consumption, PRs would require approval before all tests would be run.

PR authors would be required to run tests in their own forks to make sure that tests pass and
before the PR is approved. Alternatively Apache Pulsar committers can add a ready-to-test label
to the PR to make it eligible for testing.

This way PR authors would have CI feedback without causing resource shortage to apache/pulsar CI.

Modifications

  • stop non-doc CI builds if the PR isn't approved OR doesn't have the "ready-to-test" label.
  • Provide detailed instructions for the PR author for creating PR in their own fork by simply clicking a link.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)
    Contributor guide needs updates.

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@lhotari lhotari self-assigned this Sep 16, 2022
@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Sep 16, 2022
@lhotari lhotari changed the title [feat][CI] Add ready-to-test solution to reduce GitHub Actions resource consumption [feat][CI] Add approval solution to reduce GitHub Actions resource consumption Sep 16, 2022
@lhotari
Copy link
Member Author

lhotari commented Sep 16, 2022

Here's the latest example of the instructions that get printed as an error message and in the workflow summary:

Instructions for proceeding with the pull request:

apache/pulsar pull requests should be first tested in your own fork since the apache/pulsar CI based on
GitHub Actions has constrained resources and quota. GitHub Actions provides separate quota for
pull requests that are executed in a forked repository.

  1. Go to https://github.com/lhotari/pulsar and ensure that your branch is up to date with https://github.com/apache/pulsar
    Sync your fork if it's behind.
  2. Open a pull request to your own fork. You can use this link to create the pull request in
    your own fork:
    https://github.com/lhotari/pulsar/compare/master...lhotari:lh-pulsarci-ready-to-test?expand=1
  3. Edit the description of the pull request lh pulsarci ready to test lhotari/pulsar#83 and add the link to the PR that you opened to your own fork
    so that the reviewer can verify that tests pass in your own fork.
  4. Ensure that tests pass in your own fork. Your own fork will be used to run the tests during the PR review
    and any changes made during the review. You as a PR author are responsible for following up on test failures.
    Please report any flaky tests as new issues at https://github.com/apache/pulsar/issues
    after checking that the flaky test isn't already reported.
  5. When the PR is approved, it will be possible to restart the Pulsar CI workflow within apache/pulsar
    repository by adding a comment "/pulsarbot rerun-failure-checks" to the PR.
    An alternative for the PR approval is to add a ready-to-test label to the PR. This can be done
    by Apache Pulsar committers.
  6. When tests pass on the apache/pulsar side, the PR can be merged by a Apache Pulsar Committer.

If you have any trouble, please join the #contributor channel on Pulsar Slack to get real-time support.
Instructions for joining Pulsar Slack: https://pulsar.apache.org/community#section-discussions

To reduce resource consumption, PRs would require approval before all tests would be run.

PR authors would be required to run tests in their own forks to make sure that tests pass and
before the PR is approved. Alternatively Apache Pulsar committers can add a ready-to-test label
to the PR to make it eligible for testing.

This way PR authors would have CI feedback without causing resource shortage to apache/pulsar CI.

Please check the proposal email on dev mailing list: https://lists.apache.org/thread/gwfmxmxlhtsjn17sxxc367jcs4pcwofz
build/pulsar_ci_tool.sh Outdated Show resolved Hide resolved
build/pulsar_ci_tool.sh Outdated Show resolved Hide resolved
build/pulsar_ci_tool.sh Outdated Show resolved Hide resolved
Co-authored-by: tison <wander4096@gmail.com>
@lhotari lhotari marked this pull request as ready for review September 16, 2022 13:55
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Let's give it a try :)

build/pulsar_ci_tool.sh Outdated Show resolved Hide resolved
@lhotari
Copy link
Member Author

lhotari commented Sep 16, 2022

/pulsarbot rerun-failure-checks

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari
Copy link
Member Author

lhotari commented Sep 16, 2022

In the most recent commit 2aa2a80 to this PR, there will be an easy way to create the PR in your own fork by just clicking the link. By default the PR title will be prefixed with "[run-tests] " and the body will be "This PR is for running tests for upstream PR ${PR_URL}."

@lhotari
Copy link
Member Author

lhotari commented Sep 16, 2022

Also added a notice <!-- Before creating this PR, please ensure that the fork $FORK_REPO_URL is up to date with https://github.com/apache/pulsar --> to the generated PR body.

@nodece
Copy link
Member

nodece commented Sep 16, 2022

The ready-to-test label has been added, it works fine.

@lhotari
Copy link
Member Author

lhotari commented Sep 16, 2022

The ready-to-test label has been added, it works fine.

@nodece It also accepts the review approval as a way to allow tests to run. I added that based on feedback from @merlimat and @codelipenghui .

@lhotari
Copy link
Member Author

lhotari commented Sep 16, 2022

Latest example of the markdown (this is a result of some local testing). Please notice the "Create PR in fork for running tests" link for creating the PR.

Instructions for proceeding with the pull request:

apache/pulsar pull requests should be first tested in your own fork since the apache/pulsar CI based on
GitHub Actions has constrained resources and quota. GitHub Actions provides separate quota for
pull requests that are executed in a forked repository.

  1. Go to https://github.com/lhotari/pulsar and ensure that your branch is up to date with https://github.com/apache/pulsar
    Sync your fork if it's behind.
  2. Open a pull request to your own fork. You can use this link to create the pull request in
    your own fork:
    Create PR in fork for running tests
  3. Edit the description of the pull request lh pulsarci ready to test lhotari/pulsar#83 and add the link to the PR that you opened to your own fork
    so that the reviewer can verify that tests pass in your own fork.
  4. Ensure that tests pass in your own fork. Your own fork will be used to run the tests during the PR review
    and any changes made during the review. You as a PR author are responsible for following up on test failures.
    Please report any flaky tests as new issues at https://github.com/apache/pulsar/issues
    after checking that the flaky test isn't already reported.
  5. When the PR is approved, it will be possible to restart the Pulsar CI workflow within apache/pulsar
    repository by adding a comment "/pulsarbot rerun-failure-checks" to the PR.
    An alternative for the PR approval is to add a ready-to-test label to the PR. This can be done
    by Apache Pulsar committers.
  6. When tests pass on the apache/pulsar side, the PR can be merged by a Apache Pulsar Committer.

If you have any trouble you can get support in multiple ways:

@tisonkun
Copy link
Member

tisonkun commented Sep 18, 2022

@lhotari Perhaps we can also always show these instructions on pull requests created. All new pull requests should encounter this case.

The effect looks like pingcap/tidb#37911 (comment) and we can implement it with GitHub Actions (It's about up to 10~30 new lightweight workflow runs per day (metric)).

The reason is that the summary in check result doesn't show in the main thread of the pull request.

@momo-jun
Copy link
Contributor

CC @Anonymitaet for the contribution guide updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci doc-required Your PR changes impact docs and you will update later. ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants