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: update generic workflows #851

Conversation

asyncapi-bot
Copy link
Contributor

No description provided.

@derberg
Copy link
Member

derberg commented Oct 14, 2022

@fmvilas oh crap, now we have a problem with automerging 🤔

@fmvilas
Copy link
Member

fmvilas commented Oct 14, 2022

Any idea what's happening?

@derberg
Copy link
Member

derberg commented Oct 17, 2022

@fmvilas yeah, we now require 3 approvers

@fmvilas
Copy link
Member

fmvilas commented Oct 17, 2022

Oh damn true 😂 I just added @asyncapi-bot to an exception list. Shall we try here again?

@derberg
Copy link
Member

derberg commented Oct 17, 2022

@fmvilas oh, I had no idea we can do such a bypass

@derberg derberg closed this Oct 17, 2022
@derberg derberg reopened this Oct 17, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@fmvilas
Copy link
Member

fmvilas commented Oct 17, 2022

Me neither, I was just playing around with the UI and found it by luck 😅

@derberg
Copy link
Member

derberg commented Oct 17, 2022

merging is still blocked 🤔 and any changes in settings should work out of the box, even without closing and opening a PR

@fmvilas
Copy link
Member

fmvilas commented Oct 17, 2022

Yeah, I see. What I did is to allow asyncapi-bot to bypass having to create a PR. That's a different thing.

So my proposal then is that we don't enforce it but we still commit to the rule "manually". \cc @dalelane @char0n @smoya

@smoya
Copy link
Member

smoya commented Oct 17, 2022

So my proposal then is that we don't enforce it but we still commit to the rule "manually". \cc @dalelane @char0n @smoya

I'm fine with that!

@smoya
Copy link
Member

smoya commented Oct 17, 2022

Yeah, I see. What I did is to allow asyncapi-bot to bypass having to create a PR. That's a different thing.

So my proposal then is that we don't enforce it but we still commit to the rule "manually". \cc @dalelane @char0n @smoya

Oh, but this wouldn't help anyway, right? Automerge won't happen ever unless 2 extra approves happen (2 admins), meaning only "manual" merge by a human (via /rtm or whatever) will let the PR to be merged.

Not ideal at all IMHO. Would it make sense to rather disable this:
2022-10-17 at 15 40 35@2x
Admins could still commit to the rule of 3 approvals for all PRs, but still, those created by the bot will be automerged.
Cons: admins could merge ""accidentally"" PRs without approvals from other admins.

Tradeoffs, tradeoffs everywhere!

@fmvilas
Copy link
Member

fmvilas commented Oct 17, 2022

@smoya No, I can put it back to only 1 approver required and it would merge with no problem, like everywhere else. But we need to honor the rule of 3 approvers to merge a PR, even though it's not enforced by GitHub. This makes me think now that maybe the /rtm command could be checking for 3 approvers minimum. What do you think, @derberg?

@smoya
Copy link
Member

smoya commented Oct 17, 2022

@smoya No, I can put it back to only 1 approver required and it would merge with no problem, like everywhere else. But we need to honor the rule of 3 approvers to merge a PR, even though it's not enforced by GitHub. This makes me think now that maybe the /rtm command could be checking for 3 approvers minimum. What do you think, @derberg?

But afaik by disabling the option I mentioned still GH will enforce that, except for code owners/admins; that's the trade-off.

@derberg
Copy link
Member

derberg commented Oct 18, 2022

Yes, the action that we use for merging has a support for MERGE_REQUIRED_APPROVALS that we can set to 3.
2 disadvantages:

  • It would mean we would have to modify the workflow, and it would no longer receive updates from central location in the future
  • This would not be very transparent. People would try to merge with command, but it would not work, without providing context

Easier would be to create additional 2 bot accounts 🤣

@fmvilas
Copy link
Member

fmvilas commented Oct 18, 2022

Easier would be to create additional 2 bot accounts 🤣

LOL

It would mean we would have to modify the workflow, and it would no longer receive updates from central location in the future

Can't we make the action to take this value from a Github Secret in the repo? It would be like setting an environment variable for this repo only. We can default to 1 if this secret is not found.

This would not be very transparent. People would try to merge with command, but it would not work, without providing context

What about the command shows that it didn't merge because of that reason? I mean, it leaves a comment in the PR.

@derberg
Copy link
Member

derberg commented Oct 19, 2022

Can't we make the action to take this value from a Github Secret in the repo? It would be like setting an environment variable for this repo only. We can default to 1 if this secret is not found.

neat workaround! should work, we would have org-wide secret set to 1 and in this repo and others overwritten

What about the command shows that it didn't merge because of that reason? I mean, it leaves a comment in the PR.

yeah, I checked this action. It do not offer proper information in the output. From output we can only learn that merge did not work, but why exactly? failing tests? missing approvals? - this info is not available. Maybe they could enable it, but this will definitely require a contribution.

we could do our own query:

query { 
  repository(owner:"asyncapi", name:"spec") {
    pullRequest(number:851) {
      reviewDecision
    }
  }
}

/*
####Review Decision type####
CHANGES_REQUESTED
Changes have been requested on the pull request.

APPROVED
The pull request has received an approving review.

REVIEW_REQUIRED
A review is required before the pull request can be merged.
*/

for example this PR returns REVIEW_REQUIRED even though bot approved it. So my assumption is, that it is because API knows all 3 approvals are missing. Should be good enough to put a comment "not all approvals are in place"

maybe https://github.com/asyncapi/.github/blob/master/.github/workflows/automerge-for-humans-add-ready-to-merge-or-do-not-merge-label.yml#L29 should just be extended. We already handle "conflicts" there and post comments, so missing approvals should be handled there too I guess.

@fmvilas
Copy link
Member

fmvilas commented Oct 20, 2022

Alright, so now the question is, who volunteers to do it? 😄 I don't think it should be @derberg because he's quite busy with the conference. Any volunteers?

@derberg
Copy link
Member

derberg commented Oct 24, 2022

@fmvilas maybe just open an issue in .github and till then we will just follow kinda written agreement that we will respect minimum of 3 approvers (as we did anyway before onboarding new codeowners)

@fmvilas
Copy link
Member

fmvilas commented Oct 24, 2022

Done: asyncapi/.github#190

@derberg
Copy link
Member

derberg commented Oct 25, 2022

/rtm

1 similar comment
@derberg
Copy link
Member

derberg commented Oct 25, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 529cc06 into master Oct 25, 2022
@asyncapi-bot asyncapi-bot deleted the bot/update-global-workflow-master-64557e415f2841e194ee06e9e7a7bfbe66ff6fed branch October 25, 2022 10:19
@asyncapi-bot
Copy link
Contributor Author

🎉 This PR is included in version 3.0.0-next-major-spec.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

GreenRover pushed a commit to GreenRover/asyncapi-spec that referenced this pull request Jan 18, 2023
@asyncapi-bot
Copy link
Contributor Author

🎉 This PR is included in version 2.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants