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

Send build failure notifications to build definition .yml file synthetic sibling CODEOWNERS owners, instead of the owners of the .yml file itself. #5194

Closed
wants to merge 1 commit into from

Conversation

konrad-jamrozik
Copy link
Contributor

@konrad-jamrozik konrad-jamrozik commented Jan 24, 2023

This PR implements capability as described in:

plus does a little bit of related message output changes and code refactoring.

Copy-pasting here the relevant paragraph for convenience:

We will make the notification-configuration tool search for owners not of the pipeline definition file it originated from (e.g. /foo/ci.yml), but a made-up sibling file named e.g. PipelineOwner, e.g. /foo/PipelineOwner. This way if there is a CODEOWNERS path that matches to /foo/ci.yml it is not going to be used to determine where to route build failure notifications, but as a CODEOWNERS path that matches /foo/PipelineOwner, e.g. /foo/, will. You can find the details of this approach, and preceding discussion, here:

Testing done

I did some manual testing with C# snippets to confirm the introduced method GetSiblingFilePath behaves as expected. All other changes are done via automated IDE refactorings so should be safe.

Expected behavior change

Once this change goes in effect via PR similar to this one:

I expect there will be some changes to whom the build failure notifications are routed.

I reviewed the CODEOWNERS files targeted by the automation - build-failure-notification-subscriptions pipeline. Below I list the files and cases where owners who receive build failure notifications will change.

File Expected changes
https://github.com/Azure/azure-dev/blob/main/.github/CODEOWNERS None
https://github.com/Azure/azure-sdk-for-c/blob/main/.github/CODEOWNERS None
https://github.com/Azure/azure-sdk-for-cpp/blob/main/.github/CODEOWNERS None
https://github.com/Azure/azure-sdk-for-go/blob/main/.github/CODEOWNERS None
https://github.com/Azure/azure-sdk-for-java/blob/main/.github/CODEOWNERS aggregate-reports.yml, build
https://github.com/Azure/azure-sdk-for-js/blob/main/.github/CODEOWNERS Over 100 .yml files
https://github.com/Azure/azure-sdk-for-net/blob/main/.github/CODEOWNERS 4 .yml files
https://github.com/Azure/azure-sdk-for-python/blob/main/.github/CODEOWNERS 7 .yml files

Most likely we want to modify the CODEOWNERS files before applying the effects of this PR, to keep the recipients of build failure notifications unchanged. This can be achieved by adding relevant paths to the CODEOWNERS file.

E.g. a block like:

# PRLabel: %Mgmt
/sdk/appplatform/arm-appplatform @qiaozha @dw511214992 @MaryGao
/sdk/appplatform/ci.mgmt.yml @qiaozha @dw511214992 @MaryGao

Can be replaced with:

/sdk/appplatform/ @qiaozha @dw511214992 @MaryGao
# PRLabel: %Mgmt
/sdk/appplatform/arm-appplatform @qiaozha @dw511214992 @MaryGao
/sdk/appplatform/ci.mgmt.yml @qiaozha @dw511214992 @MaryGao

Additional design notes

Currently I assume the proposed PipelineOwnerSyntheticFileName is not present in the repository nor in CODEOWNERS file. But we might actually want to consider adding this file path to CODEOWNERS a valid approach, to fine-tune who gets the build notifications. Then the example above would become, for example:

# PRLabel: %Mgmt
/sdk/appplatform/arm-appplatform @qiaozha @dw511214992 @MaryGao
/sdk/appplatform/__PipelineOwner__ @qiaozha @dw511214992 @MaryGao
/sdk/appplatform/ci.mgmt.yml @qiaozha @dw511214992 @MaryGao

However, in such case, we would need to add special case that such file should not actually exist, in our validation spec:

…tic sibling "PipelineOwner" CODEOWNERS owners, instead of the owners of the .yml file itself.
@konrad-jamrozik konrad-jamrozik added the Central-EngSys This issue is owned by the Engineering System team. label Jan 24, 2023
@konrad-jamrozik konrad-jamrozik self-assigned this Jan 24, 2023
@konrad-jamrozik konrad-jamrozik requested a review from a team as a code owner January 24, 2023 04:49
@konrad-jamrozik konrad-jamrozik changed the title Send build failure notifications to build definition .yml file synthetic sibling "PipelineOwner" CODEOWNERS owners, instead of the owners of the .yml file itself. Send build failure notifications to build definition .yml file synthetic sibling CODEOWNERS owners, instead of the owners of the .yml file itself. Jan 24, 2023
@konrad-jamrozik
Copy link
Contributor Author

konrad-jamrozik commented Jan 24, 2023

@weshaggard I need your input before I can proceed with this PR. Please see the description, sections "Expected behavior change" and "Additional design notes".

Should I go ahead with adjusting all the language repo CODEOWNERS files to ensure this PR doesn't change any recipients of build failure notifications? If so, which approach would you prefer? I like the approach that adds the synthetic file name to CODEOWNERS just above the .yml file, like: /sdk/appplatform/__PipelineOwner__.

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

As I mentioned in the original PR lets not go after this right now. @benbp will look into other workflows for monitoring all the yml files in the repo.

@konrad-jamrozik
Copy link
Contributor Author

konrad-jamrozik commented Jan 25, 2023

Closing this PR per Wes' comment above. Instead, I submitted a PR that incorporates refactorings made in this PR, and more:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants