Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jan 11, 2021

What changes were proposed in this pull request?

This is kind of a followup of #31104 but I decided to track it separately with a separate JIRA.

Currently the jobs are being canceled in main repo branches. If a commit is merged, for example, to master branch before the test finishes, it cancels the previous builds. This is a problem because we cannot, for example, detect logical conflict properly. We should only cancel the jobs in PRs:

Screen Shot 2021-01-11 at 3 22 24 PM

This PR proposes to don't do this in the main repo branch commits but only do it in PRs.

Why are the changes needed?

  • To keep the test coverage
  • To run the test in the synced master branch instead of relying on the builds made in each PR with an outdated master branch
  • To detect test failures from logical conflicts from merging two conflicting PRs at the same time.

Does this PR introduce any user-facing change?

No, dev-only.

How was this patch tested?

I manually tested in

I added Yi Wu as a co-author since he helped verifying the current fix in the PR above.

I checked that it does not cancel in the main repo branch:

Screen Shot 2021-01-11 at 3 58 52 PM

I checked it cancels in PRs:

Screen Shot 2021-01-11 at 3 58 45 PM

@github-actions github-actions bot added the INFRA label Jan 11, 2021
@HyukjinKwon
Copy link
Member Author

cc @mik-laj and @dongjoon-hyun FYI

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Nice!


jobs:
cancel-duplicate-workflow-runs:
if: github.event.workflow_run.event == 'pull_request'
Copy link
Member Author

Choose a reason for hiding this comment

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

@dongjoon-hyun, I referred to https://securitylab.github.com/research/github-actions-preventing-pwn-requests and found this way is slightly better because it's at least easier to understand. Likewise, I tested it in my fork, and updated PR description.

@HyukjinKwon
Copy link
Member Author

Let me merge this in to recover the test coverage. Let's see if it works out of the box in the main repo too.

@HyukjinKwon
Copy link
Member Author

Merged to master.

@HyukjinKwon
Copy link
Member Author

Seems working fine:
Screen Shot 2021-01-11 at 4 38 33 PM
Screen Shot 2021-01-11 at 4 38 58 PM

@SparkQA
Copy link

SparkQA commented Jan 11, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38498/

@SparkQA
Copy link

SparkQA commented Jan 11, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38498/

@SparkQA
Copy link

SparkQA commented Jan 11, 2021

Test build #133909 has finished for PR 31121 at commit 31cd9f7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 11, 2021

Test build #133911 has finished for PR 31121 at commit 4b32e9f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants