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

[fix][workflow] Support concurrency group and reusable workflow files in docbot #16060

Merged
merged 2 commits into from
Jun 15, 2022

Conversation

maxsxu
Copy link
Contributor

@maxsxu maxsxu commented Jun 14, 2022

Fixes #16046

Motivation

Multiple workflows can be running concurrently while labeled event triggered instantly after opened event before the docbot workflow is running.

This scenario will cause an inconsistent state of action and confused results.

Modifications

Added concurrency

  • concurrency.group means workflow using the same concurrency group will be pending
  • concurrency.cancel-in-progress will cancel any currently running job or workflow in the same concurrency group. This option can resolve the inconsistent state issue.

Tests

The demo can be found at open-github#12

docbot-concurrency

And the workflow status

image

Multi workflows can be triggered while opened then labeled instantly,
This scenario will cause inconsistent state of action.

Signed-off-by: Max Xu <maxs.xu@gmail.com>
@lhotari
Copy link
Member

lhotari commented Jun 14, 2022

btw. please also change uses: apache/pulsar-test-infra/docbot@master to uses: ./docbot . That should be done since the apache/pulsar-test-infra is already checked out.

Syntax for uses

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

see #16060 (comment) . I think it makes sense to address it in the same PR.

@maxsxu
Copy link
Contributor Author

maxsxu commented Jun 14, 2022

btw. please also change uses: apache/pulsar-test-infra/docbot@master to uses: ./docbot . That should be done since the apache/pulsar-test-infra is already checked out.

Syntax for uses

Thanks for this great suggestion! Totally agree with this method.

Just a detail: If we address it in the same PR, should we change the PR title? @lhotari

@lhotari
Copy link
Member

lhotari commented Jun 14, 2022

Just a detail: If we address it in the same PR, should we change the PR title?

yes that makes sense.

@maxsxu maxsxu changed the title [fix][workflow] Support concurrency group in docbot [fix][workflow] Support concurrency group and reusable workflow files in docbot Jun 14, 2022
Signed-off-by: Max Xu <maxs.xu@gmail.com>
@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. workflow labels Jun 15, 2022
@Anonymitaet Anonymitaet merged commit 39218f4 into apache:master Jun 15, 2022
@maxsxu maxsxu deleted the fix/docbot branch June 15, 2022 01:40
@@ -46,7 +50,7 @@ jobs:
go-version: 1.18

- name: Labeling
uses: apache/pulsar-test-infra/docbot@master
uses: ./docbot
Copy link
Member

Choose a reason for hiding this comment

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

Why we cannot use apache/pulsar-test-infra/docbot@master in this case like pulsarbot?

cc @maxsxu

Copy link
Member

Choose a reason for hiding this comment

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

@tisonkun I've explained that in this comment: #16060 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

If we adopt docker container action as described in apache/pulsar-test-infra#58, we may need not to checkout the code as well as setup-go, just use the action itself ><

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The new Docbot spams PRs with multiple comments
4 participants