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

[Bug]: GitHub Action Phrase Issues #28320

Closed
15 tasks
Abacn opened this issue Sep 5, 2023 · 9 comments
Closed
15 tasks

[Bug]: GitHub Action Phrase Issues #28320

Abacn opened this issue Sep 5, 2023 · 9 comments
Assignees
Labels

Comments

@Abacn
Copy link
Contributor

Abacn commented Sep 5, 2023

What happened?

Create a persistent issue for GitHub Action phrase implementations. Update the current status in comments below

Issue Priority

Priority: 2 (default / most bugs should be filed as P2)

Issue Components

  • Component: Python SDK
  • Component: Java SDK
  • Component: Go SDK
  • Component: Typescript SDK
  • Component: IO connector
  • Component: Beam examples
  • Component: Beam playground
  • Component: Beam katas
  • Component: Website
  • Component: Spark Runner
  • Component: Flink Runner
  • Component: Samza Runner
  • Component: Twister2 Runner
  • Component: Hazelcast Jet Runner
  • Component: Google Cloud Dataflow Runner
@Abacn
Copy link
Contributor Author

Abacn commented Sep 5, 2023

Current status

  • The comment phrase can work and run the workflow, but there are at least following problem

- Another PR comment, regardless what content it is, will cancel the ongoing phrase triggered run for another job
this essentially let only one phrase triggered workflow can be run at one time. This would be a blocker for retire Jenkins because mass_commit won't work

  • If not triggered by commit, phrase triggered job won't update its status to the PR.
    While it does not affect functionality, this introduce inconvenience when developing postcommit / need test with postcommits

Example PR displaying these issues: #28316

@volatilemolotov
Copy link
Contributor

Seems like we need to think about the PostCommit nature to solve it correctly.

Not sure at this point what is the best option but ill take the issue and report what is going on and how to continue.

@volatilemolotov
Copy link
Contributor

.take-issue

@volatilemolotov
Copy link
Contributor

volatilemolotov commented Sep 6, 2023

Ok so the first issue

Another PR comment, regardless what content it is, will cancel the ongoing phrase triggered run for another job
this essentially let only one phrase triggered workflow can be run at one time. This would be a blocker for retire
Jenkins because mass_commit won't work

Is due to concurrency groups added:

concurrency:
  group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.sha || github.head_ref || github.ref }}-${{ github.event.sender.login }}-${{ github.event.schedule }}'
  cancel-in-progress: true

It can be fixed by adding ${{ github.event.schedule || github.event.comment.created_at }} in the final part. Note that schedule event does not affect this because there is OR in the expression.
Now its just a decision to either use:
github.event.comment.body - Which will make groups based on Job name. This way if you run 2 jobs with 2 comments the newer will cancel the older. Is this wanted behaviour?
github.event.comment.id - Which as per https://docs.github.com/en/webhooks/webhook-events-and-payloads#issue_comment is a unique comment id and will create a group of its own meaning that 2 comments will not cancel eachother out. There is also created_at that can be used if these ids are not unique but I cannot find the exact spec of created_at. Im going to guess its seconds.

@Abacn let me know what you think would be the preferred approach.

@Abacn
Copy link
Contributor Author

Abacn commented Sep 6, 2023

Thanks, github.event.comment.body sounds good. Does "irrelevant comment will cancel the previous one" mean ${{ github.workflow }} never worked?

Update:

did another experimental: https://github.com/apache/beam/actions/runs/6101642034

The current group name is 'PreCommit Whitespace @ 9954d3e-Abacn-'
where 9954d3ed5df68b3fba2cfc67f6a89596e018c475 is the sha for current master.

If add "github.event.comment.body", it won't be cancelled by irrelevant comment, but it can still be cancelled by the same comment in another irrelevant PR (saying I opened two PRs and use the same phrase trigger). Let me also think a little bit what it should be

@Abacn
Copy link
Contributor Author

Abacn commented Sep 6, 2023

how about

'${{ github.workflow }} @ ${{ github.event.issue.number || github.event.pull_request.head.label || github.sha || github.head_ref || github.ref }}-${{ github.event.schedule || github.event.comment.body }}' ?

this is because the second part ${{ github.event.pull_request.head.label || github.sha || github.head_ref || github.ref }} is observed to be always head sha for pull request; and we do not care about sender - just same PR should have only one run on one workflow

per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment-on-issues-only-or-pull-requests-only - ${{ github.event.issue.number }} is available in issue_comment event

@volatilemolotov
Copy link
Contributor

${{ github.event.sender.login }} is needed for dispatch

@Abacn
Copy link
Contributor Author

Abacn commented Sep 7, 2023

Then how about '${{ github.workflow }} @ ${{ github.event.issue.number || github.event.pull_request.head.label || github.sha || github.head_ref || github.ref }}-${{ github.event.schedule || github.event.comment.body || github.event.sender.login}}' ?

@volatilemolotov
Copy link
Contributor

Created a draft: #28373
I agree with the approach and some limited testing shows it should be correct
If we agree I will move from draft and lets merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants