Skip to content

add review comment to sb files #48566

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

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

oleksandr-didyk
Copy link
Contributor

@oleksandr-didyk oleksandr-didyk commented Jun 1, 2023

Add review comment to SourceBuild* files

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Add comment to SourceBuild* files asking to include source-build team in PRs.

Description

Contributes to dotnet/source-build#3435

Adds comments to source-build files asking for the inclusion of the source-build team in PRs that alter SourceBuild* files. Non-reviewed changes could potentially cause issues down the line, be it in the downstream repos or the product build (as has happened in the past, see dotnet/source-build#3435 (comment))

@oleksandr-didyk oleksandr-didyk requested review from a team and wtgodbe as code owners June 1, 2023 13:55
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jun 1, 2023
@ghost
Copy link

ghost commented Jun 1, 2023

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

@@ -1,3 +1,5 @@
<!-- Whenever altering this or other Source Build files, please include @dotnet/source-build-internal as a reviewer. -->
Copy link
Member

Choose a reason for hiding this comment

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

If this is for file-level review perhaps we could use GH owners file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is for file-level review perhaps we could use GH owners file?

Initially wanted to add both the comment and the CODEOWNERS entry, but for a team to be added as a code owner it needs to have write permissions to the repo and I wanted to verify first if this is something we would need. I should've mentioned that in the description though, my bad, forgot to edit it.

If you are OK with granting write permissions to dotnet/source-build-internal, I will add the CODEOWNERS entry.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, probably not.

Copy link
Contributor Author

@oleksandr-didyk oleksandr-didyk Jun 7, 2023

Choose a reason for hiding this comment

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

Then unfortunately the comment is our best bet.

We will monitor how effective they are and will of course change / adapt based on performance. But for now this is a relatively cheap / easy option

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with giving dotnet/source-build-internal write access - we already have 500+ users with that level of access (everyone in dotnet/npt). Any objections @dotnet/aspnet-admins?

Copy link
Member

Choose a reason for hiding this comment

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

Ping @dotnet/aspnet-admins

Copy link
Member

Choose a reason for hiding this comment

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

@danmoseley, @mkArtak any opposition to giving dotnet/sorce-build-internal write access to the repo so that we can add them to Codeowners for this file?

Copy link
Member

Choose a reason for hiding this comment

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

@danmoseley, @mkArtak any opposition to giving dotnet/sorce-build-internal write access to the repo so that we can add them to Codeowners for this file?

if the owners of dotnet/source-build-internal believe that is reasonable, sure

Copy link
Member

Choose a reason for hiding this comment

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

@oleksandr-didyk I've given dotnet/source-build-internal write access to the repo - feel free to update the PR to add the to CODEOWNERS for this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, added the entry for the file

As for altering the comment to make it more visible (discussed below) - given the valid CODEOWNERS entry there is less pressure on the comment so I opted to keeping it the same as in other repositories

@amcasey
Copy link
Member

amcasey commented Jun 12, 2023

Copyright headers have trained me to ignore the first two lines of most files. Personally, I'd make a bigger, bolder comment with multiple lines and some SHOUTING.

@oleksandr-didyk
Copy link
Contributor Author

Copyright headers have trained me to ignore the first two lines of most files. Personally, I'd make a bigger, bolder comment with multiple lines and some SHOUTING.

Given the line count for the files I wanted to be less in-your-face-ish about it + rely on unaware contributors scanning the file to find any help when editing something they are unaware of.

Nevertheless, you are right that it could be more visible. Thank you for the comment, I'll think of how to highlight it better.

@oleksandr-didyk oleksandr-didyk force-pushed the add-sb-review-comment branch from 2827ffa to 1354e6e Compare June 15, 2023 14:28
@wtgodbe wtgodbe enabled auto-merge (squash) June 15, 2023 16:15
@wtgodbe wtgodbe merged commit 2971950 into dotnet:main Jun 15, 2023
@ghost ghost added this to the 8.0-preview6 milestone Jun 15, 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