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

add review comment to sb files #87003

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

oleksandr-didyk
Copy link
Contributor

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))

@ghost
Copy link

ghost commented Jun 1, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

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))

Author: oleksandr-didyk
Assignees: oleksandr-didyk
Labels:

area-Infrastructure-libraries

Milestone: -

@@ -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.

Instead of adding this comment, add lines with this alias for these files to

https://github.com/dotnet/runtime/blob/main/.github/CODEOWNERS

It will ensure that you are automatically included on PRs that modify these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Hmm, I did not know about the write access requirements. I have looked around and we do not have prior art of giving repo write access to team aliases. The current code owner entries that point to feature team aliases (like @dotnet/jit-contrib) are not effective.

We have automation setup by @terrajobst that monitors who has write access to the repos. I am not sure how it is going to react to granting write access to feature team aliases. This is something that would be best discussed over email first. You can start email thread about this with .NET OSS Admins.

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, will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The e-mail thread unfortunately is taking a bit longer then expected.

Would it make sense from your perspective to merge the PR with the comments only and return to the subject of CODEOWNERS entry later? Thank you.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 1, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 2, 2023
@@ -1,4 +1,6 @@
<!-- 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.

Suggested change
<!-- Whenever altering this or other Source Build files, please include @dotnet/source-build-internal as a reviewer. -->
<!-- Whenever altering this file, please include @dotnet/source-build-internal as a reviewer. -->

"other Source Build files" is ambiguous. It is not clear what those files are from the comment. Is it just the SourceBuild.props 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.

True, thank you for pointing it out.

Currently there are two (SourceBuild.props and SourceBuildPreBuiltBaseline.xml), but there might be more in the future.
Probably changing it to Whenever altering this or other SourceBuild* files would be more useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved the comment a bit to better characterize which exact files we are referring to.

@jkotas jkotas merged commit ec07eb4 into dotnet:main Jun 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants