Extract Contacts class + related refactorings to NotificationConfigurator class.#5214
Merged
konrad-jamrozik merged 2 commits intomainfrom Jan 26, 2023
Merged
Conversation
konrad-jamrozik
added
the
Central-EngSys
konrad-jamrozik
force-pushed
the
users/kojamroz/pipeline_owner_refactor
branch
2 times, most recently
from
08ae91f to
4f89a25
Compare
…nfigurator class.
konrad-jamrozik
force-pushed
the
users/kojamroz/pipeline_owner_refactor
branch
from
4f89a25 to
937bd1f
Compare
benbp
reviewed
Jan 25, 2023
tools/notification-configuration/notification-creator/Contacts.cs
Outdated
Show resolved
Hide resolved
benbp
reviewed
Jan 25, 2023
tools/notification-configuration/notification-creator/Contacts.cs
Outdated
Show resolved
Hide resolved
benbp
reviewed
Jan 25, 2023
tools/notification-configuration/notification-creator/Contacts.cs
Outdated
Show resolved
Hide resolved
konrad-jamrozik
force-pushed
the
users/kojamroz/pipeline_owner_refactor
branch
from
8d072ff to
13a1439
Compare
konrad-jamrozik
force-pushed
the
users/kojamroz/pipeline_owner_refactor
branch
4 times, most recently
from
38b72de to
8c28493
Compare
konrad-jamrozik
force-pushed
the
users/kojamroz/pipeline_owner_refactor
branch
from
8c28493 to
e97ce38
Compare
benbp
approved these changes
Jan 26, 2023
This was referenced Jan 26, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes made
This PR improves logged messages and refactors
NotificationConfiguratorclass logic related to obtaining list of contacts to send notifications to based on CODEOWNERS file from repository to which given pipeline pertains.The main change is extraction of
Contactsclass fromNotificationConfiguratorclass, with entry method beingContacts.GetFromBuildDefinitionRepoCodeowners(BuildDefinition buildDefinition).I also made related improvements to log messages and naming in surrounding code. Most notably, I made the naming more consistent and got rid of some synonyms or mismatched names, like
BuildDefinitionbeing namedPipeline.Test plan
Once this PR is merged and package published, I will run the
automation - build-failure-notification-subscriptionspipeline from a branch that uses the new package and report how it went here. I do not expect any issues as the PR is composed only of automated refactoring and logged messages updates.If there are problems, I will re-run the pipeline from
mainto restore the notification setup.Update 1/26/2023: here is the build doing the test.
The pipeline run looks good, no issues.
Context
This PR originated by salvaging refactored code from this PR:
.ymlfile synthetic sibling CODEOWNERS owners, instead of the owners of the.ymlfile itself. #5194And significantly expanding upon it.
In general, I have bunch of related work upcoming pertaining to the logic modified in this PR, hence I am working on improving the code. Related issues:
notification-configuration#5089