Skip to content

Conversation

@mkArtakMSFT
Copy link
Contributor

This label has been removed as part of the effort to clean-up the labels in the repo. So these rules don't work anymore. During the conversation with some team members, it sounded like the current behavior is preferable.

This label has been removed as part of the effort to clean-up the labels in the repo. So these rules don't work anymore. During the conversation with some team members it sounded like the current behavior is preferable.
@mkArtakMSFT mkArtakMSFT added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 13, 2023
@mkArtakMSFT mkArtakMSFT requested review from a team and wtgodbe as code owners May 13, 2023 19:01
@ghost
Copy link

ghost commented May 13, 2023

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@joperezr
Copy link
Member

Not exactly related but what about [Infrastructure PRs] Add area-infrastructure label to dependency update Pull Requests Rule? I noticed it is approving the PRs as well, should we remove that action?

@joperezr
Copy link
Member

Same question for task [Infrastructure PRs] Add area-infrastructure label to auto-merge Pull Requests

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Left a small question regarding two other tasks, but otherwise this looks good, thanks @mkArtakMSFT!

@mkArtakMSFT
Copy link
Contributor Author

mkArtakMSFT commented May 15, 2023

Not exactly related but what about [Infrastructure PRs] Add area-infrastructure label to dependency update Pull Requests Rule? I noticed it is approving the PRs as well, should we remove that action?

There was a reason we did this in the first place. Do we have reasons to believe that we should back up on this decision?
@wtgodbe what do you think?

@joperezr
Copy link
Member

I'm assuming the reason might have been that in order for dependency flow PRs to get auto merged they required an approval? If that's the case, now that we are removing the auto-merge capability for those, then it probably makes sense to also remove the auto-approval.

@mkArtakMSFT
Copy link
Contributor Author

Let's see what @wtgodbe has to say about this.

@wtgodbe
Copy link
Member

wtgodbe commented May 16, 2023

I think it's fine to keep the auto-approval, that keeps the build-ops person's task more mechanical (check the PR is green, merge the PR). And I like keeping the auto-label for them too, it's accurate.

@mkArtakMSFT
Copy link
Contributor Author

@joperezr I'm merging this change as is. I'm happy to continue the conversation and if we decide that more rules should be removed, I'd send another PR out.

@mkArtakMSFT mkArtakMSFT merged commit 2dde698 into main May 16, 2023
@mkArtakMSFT mkArtakMSFT deleted the mkArtakMSFT/no-auto-merge branch May 16, 2023 16:25
@ghost ghost added this to the 8.0-preview5 milestone May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants