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

ci: let autoupdate.yml run on pull_request #146

Closed
wants to merge 1 commit into from

Conversation

smoya
Copy link
Member

@smoya smoya commented Mar 21, 2022

Description

I’m thinking that the autoupdate.yml action that keeps release branches up-to-date with the destination branch (e.g. master ) should also run on creating a PR and not only on push event.
https:/asyncapi/spec/blob/master/.github/workflows/autoupdate.yml#L13-L15

I recently created few PR’s in order to keep the release branches (2022-04-release) up-to-date with master branch . But I don’t want to wait until someone else pushes changes on those release branches. Instead, I would like the bot to update them ASAP. And this is not possible since the action is triggered only by the push event.

Does it make sense to also trigger it on pull_request event?
If not, please discard this PR.

@derberg
Copy link
Member

derberg commented Mar 21, 2022

@smoya I'm concerned that this workflow will run on any PR event to master branch. So a lot of triggers with no effect.

maybe better we should come up with some command like /update that would call proper workflow that updates the base branch of PR where /update comment was made?

somehow related to #115

@smoya
Copy link
Member Author

smoya commented Mar 21, 2022

We could also let the action to be triggered under demand with the workflow_dispatch event.

@derberg
Copy link
Member

derberg commented Mar 21, 2022

@smoya just keep in mind only codeowners, folks with maintainers rights can trigger such workflow event

@smoya
Copy link
Member Author

smoya commented Mar 21, 2022

@smoya just keep in mind only codeowners, folks with maintainers rights can trigger such workflow event

yikes! Yeah, that is not ideal :(

@KhudaDad414
Copy link
Member

@derberg If I understand it correctly, we need to do the /update command in cascade. like if we have master -> 2022-04-release -> some-patch, first we have to update 2022-04-release with master, then we can update some-patch with 2022-04-release.
Getting changes from master to 2022-04-release is a little bit tricky.

@derberg
Copy link
Member

derberg commented Mar 22, 2022

@KhudaDad414 sorry, I didn't get it. Why Getting changes from master to 2022-04-release is a little bit tricky. ?
This is what autoupdate-action is going. Basically merging master into the base branch, as long as there are no conflicts of course.

Maybe we can even use the same action, but triggered not by push but by PR comment with /update, no?

@KhudaDad414
Copy link
Member

KhudaDad414 commented Mar 22, 2022

@derberg sorry, I meant updating the whole chain of branches from the leaf branch PR would be hard but as I see now, we don't need to do it.
I propose we do it like this:

  1. create a new label called auto-update for PRs.
  2. create a new command called /auto-update or /update, which adds the auto-update label to the PR when invoked.
  3. add auto-update alongside autoapprove here
  4. update autoupdate.yml so it runs not only on push but on label: created as well, which updates all the respective PRs that have auto-update label.

this way we separate the function of autoapprove and auto-update labels. PRs like asyncapi/parser-js#501 will be kept up to date in their entire life and /auto-update can be used for #115 as well.

what do you think?

@derberg
Copy link
Member

derberg commented Mar 22, 2022

if it is label based then it means it will work only once, right? and it won't be much useful for #115 ? cause sometimes you might need to request update more than once

@KhudaDad414
Copy link
Member

KhudaDad414 commented Mar 22, 2022

if it is label based then it means it will work only once, right?

@derberg Not necessarily in this case. since autoupdate.yml will run on push event as well, having auto-update label is the condition for being updated. In theory, I can call /auto-update on any PR and the PR's branch will be kept updated till it is merged. so no need to request updates more than once.

@smoya
Copy link
Member Author

smoya commented Mar 22, 2022

4. update autoupdate.yml so it runs not only on push but on label: created as well, which updates all the respective PRs that have auto-update label.

Afaik, the label: created event is triggered only when you create the label in the repository and not when you add the label to a PR. In fact, their documentation says:

If you want to run your workflow when a label is added to or removed from an issue, pull request, or discussion, use the labeled or unlabeled activity types for the issues, pull_request, pull_request_target, or discussion events instead.
Source: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#label

@@ -17,6 +17,13 @@ on:
- 'dependabot/**'
- 'bot/**'
- 'all-contributors/**'
pull_request:
types: [opened, reopened, synchronize, ready_for_review]
Copy link
Member

Choose a reason for hiding this comment

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

What is the synchronize PR type for? 🤔

@quetzalliwrites
Copy link
Member

  • create a new label called auto-update for PRs.
  • create a new command called /auto-update or /update

I like @smoya's idea and what @KhudaDad414 suggests here:

  • create a new label called auto-update for PRs.
  • create a new command called /auto-update or /update

@derberg
Copy link
Member

derberg commented Mar 28, 2022

closing in favour of #149

@derberg derberg closed this Mar 28, 2022
@smoya smoya deleted the autoupdate branch March 28, 2022 10:27
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