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.

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

Merged
merged 2 commits into from
Jun 15, 2023
Merged

add review comment to sb files #48566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
405218d
add review comment to sb files
oleksandr-didyk Jun 1, 2023
1354e6e
add CODEOWNERS entry for source-build
oleksandr-didyk Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
/eng/common/ @dotnet-maestro-bot
/eng/Versions.props @dotnet-maestro-bot @dotnet/aspnet-build @wtgodbe
/eng/Version.Details.xml @dotnet-maestro-bot @dotnet/aspnet-build @wtgodbe
/eng/SourceBuild* @dotnet/source-build-internal
/src/Caching/ @captainsafia @halter73 @mgravell
/src/Caching/**/PublicAPI.*Shipped.txt @dotnet/aspnet-api-review @captainsafia @halter73 @mgravell
/src/Components/ @dotnet/aspnet-blazor-eng
Expand Down
2 changes: 2 additions & 0 deletions eng/SourceBuild.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
<!-- Whenever altering this or other Source Build files, please include @dotnet/source-build-internal as a reviewer. -->
Copy link
Member

@mitchdenny mitchdenny Jun 5, 2023

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

@oleksandr-didyk oleksandr-didyk Jun 5, 2023

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

@mitchdenny mitchdenny Jun 7, 2023

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 •
edited
Loading

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

@wtgodbe wtgodbe Jun 7, 2023

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

@amcasey amcasey Jun 12, 2023

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

@wtgodbe wtgodbe Jun 13, 2023

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

@danmoseley danmoseley Jun 13, 2023

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

@wtgodbe wtgodbe Jun 13, 2023

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

@oleksandr-didyk oleksandr-didyk Jun 15, 2023

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


<Project>

<PropertyGroup>
Expand Down
3 changes: 3 additions & 0 deletions eng/SourceBuildPrebuiltBaseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
<!-- Whenever altering this or other Source Build files, please include @dotnet/source-build-internal as a reviewer. -->
<!-- See aka.ms/dotnet/prebuilts for guidance on what pre-builts are and how to eliminate them. -->

<UsageData>
<IgnorePatterns>
<UsagePattern IdentityGlob="Microsoft.SourceBuild.Intermediate.*/*" />
Expand Down