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

Update submodule to latest master in microsoft/main #1251

Merged

Conversation

microsoft-golang-bot
Copy link
Collaborator

Hi! I'm a bot, and this is an automatically generated upstream sync PR. 🔃

After submitting the PR, I will attempt to enable auto-merge in the "merge commit" configuration.

For more information, visit sync documentation in microsoft/go-infra.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Auto-approving.

@karianna
Copy link
Member

Hmm, it seems this bot always auto approves even before CI has a chance to check things, should we change that behaviour?

@dagood
Copy link
Member

dagood commented Jun 17, 2024

Hmm, it seems this bot always auto approves even before CI has a chance to check things, should we change that behaviour?

Do you have some ideas why we might want to change it, or concerns?

@gdams
Copy link
Member

gdams commented Jun 19, 2024

Hmm, it seems this bot always auto approves even before CI has a chance to check things, should we change that behaviour?

Do you have some ideas why we might want to change it, or concerns?

I guess it might not be a bad idea to have it only approve it if all CI is green? It doesn't bother me that much though

@dagood
Copy link
Member

dagood commented Jun 19, 2024

I guess it might not be a bad idea to have it only approve it if all CI is green?

I don't think this behavior would be bad, but I also don't see any reason it would be better. I don't expect human reviewers to wait for CI to finish either.

@dagood
Copy link
Member

dagood commented Jun 21, 2024

CI failure is an upstream change: filed golang/go#68121.

@karianna
Copy link
Member

I guess it might not be a bad idea to have it only approve it if all CI is green?

I don't think this behavior would be bad, but I also don't see any reason it would be better. I don't expect human reviewers to wait for CI to finish either.

I'll ask a different Q - what is the bot checking for before it gives the approval?

… Enable arm64 assembler tests for cross-compiler builds
@microsoft-golang-review-bot microsoft-golang-review-bot merged commit 0385f4c into microsoft/main Jun 24, 2024
23 checks passed
@dagood
Copy link
Member

dagood commented Jun 24, 2024

what is the bot checking for before it gives the approval?

It doesn't check anything. For context, microsoft-golang-review-bot and microsoft-golang-bot are separate accounts, but they are not independent in any way. A pipeline creates the PR with one account's PAT and (during the same command) uses the other account's PAT to approve it and turn on auto-merge. GitHub makes sure the CI passes before automerge goes ahead, because the branch is protected.

We have branch protection requiring a review approval--the goal is making dev PRs comfortable. The microsoft-golang-review-bot approvals are just a way to make auto-sync work without removing/bypassing branch protection.

@karianna
Copy link
Member

what is the bot checking for before it gives the approval?

It doesn't check anything. For context, microsoft-golang-review-bot and microsoft-golang-bot are separate accounts, but they are not independent in any way. A pipeline creates the PR with one account's PAT and (during the same command) uses the other account's PAT to approve it and turn on auto-merge. GitHub makes sure the CI passes before automerge goes ahead, because the branch is protected.

We have branch protection requiring a review approval--the goal is making dev PRs comfortable. The microsoft-golang-review-bot approvals are just a way to make auto-sync work without removing/bypassing branch protection.

Cool, thanks for the context!

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.

6 participants