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.

Sign up for GitHub

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

Jump to bottom

Pull Request Etiquette and Best Practices #5953

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Pull Request Etiquette and Best Practices #5953

wants to merge 5 commits into from

Conversation

donnie-msft
Copy link
Contributor

Copied directly from the Azure SDK team's repo. I read their contributing document recently and thought it seemed appropriate that they call out PR Review etiquette. We have been wanting to improve our PR etiquette for years and I believe writing down guidelines is a step to achieving common understanding.

@donnie-msft donnie-msft requested a review from a team as a code owner August 6, 2024 04:18
@donnie-msft
Copy link
Contributor Author

Recreated PR as I mistakenly left the default branch name which didn't trigger CI.
Thanks @kartheekp-ms for the feedback - addressed both suggestions.
Thanks @jeffkl for the approval, please see the new PR.

zivkan
zivkan reviewed Aug 6, 2024
View reviewed changes
docs/workflow.md Outdated Show resolved Hide resolved
@donnie-msft donnie-msft mentioned this pull request Aug 6, 2024
Merged
nkolev92
nkolev92 reviewed Aug 6, 2024
View reviewed changes
docs/workflow.md Outdated Show resolved Hide resolved
@aortiz-msft
Copy link
Contributor

Team review: We do not agree on a few of the points being suggested here. We should have a team discussion on these first.

@donnie-msft
Copy link
Contributor Author

Team review: We do not agree on a few of the points being suggested here. We should have a team discussion on these first.

Yes, I already had this in the team meeting agenda. If you can share your concern here, please do.

donnie-msft
donnie-msft commented Aug 8, 2024
View reviewed changes
Copy link
Contributor Author

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Proposing changes based on team meeting

docs/workflow.md Outdated Show resolved Hide resolved
docs/workflow.md Outdated Show resolved Hide resolved
@donnie-msft @nkolev92
Address feedback
9268898 
Co-authored-by: Nikolche Kolev <[email protected]>
@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 16, 2024
@donnie-msft
Copy link
Contributor Author

Not stale, just awaiting approvals

@dotnet-policy-service dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 19, 2024
@donnie-msft donnie-msft requested review from nkolev92, zivkan and a team and removed request for nkolev92 and zivkan August 19, 2024 20:00
nkolev92
nkolev92 approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I think people may want to rereview in a team meeting, but LGTM.

@donnie-msft
Copy link
Contributor Author

Merging is blocked because @aortiz-msft would like to discuss this again in the 9/12 Team Meeting.

@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 4, 2024
@donnie-msft
Copy link
Contributor Author

Bump #5953 (comment)

@dotnet-policy-service dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 4, 2024
@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 11, 2024
@donnie-msft
Copy link
Contributor Author

Bump #5953 (comment)

@dotnet-policy-service dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 11, 2024
@donnie-msft
Copy link
Contributor Author

Discussed in Team Meeting 9/12. Let me know if there are suggested changes to make here.

@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 20, 2024
@donnie-msft
Copy link
Contributor Author

Bump #5953 (comment)

@dotnet-policy-service dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 20, 2024
@nkolev92
Copy link
Member

I'd merge this :) 5 approvals on latest (including a double one by me :D )

@donnie-msft
Copy link
Contributor Author

I'd merge this :) 5 approvals on latest (including a double one by me :D )

Sure! @aortiz-msft sound good?

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@jgonz120 jgonz120 jgonz120 approved these changes

@jeffkl jeffkl jeffkl approved these changes

@kartheekp-ms kartheekp-ms kartheekp-ms approved these changes

@Nigusu-Allehu Nigusu-Allehu Nigusu-Allehu approved these changes

@zivkan zivkan Awaiting requested review from zivkan

Assignees
No one assigned
Labels
Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@donnie-msft @aortiz-msft @nkolev92 @zivkan @jgonz120 @jeffkl @kartheekp-ms @Nigusu-Allehu