Refactor MonitorRunner

[qreshi]

Signed-off-by: Mohammad Qureshi [email protected]

Issue #, if available: #86

Description of changes:
Refactor MonitorRunner to organize code in preparation for Bucket-Level Monitor changes.

CheckList:
[x] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[qreshi]

Given the way the commits/files were re-organized to prepare merging Bucket-Level Alerting into main, the tests/build are expected to fail until we get to the PR that updates the MonitorRunner logic. So I'll be overriding the approval workflow failures as long as the PRs are approved by the reviewers until we've caught up to the end of the dev branch. If at that point, there are any build failures, I'll fix them to pass (although I confirmed the head of the dev branch was stable before beginning to push these PRs).

[adityaj1107]

Given the way the commits/files were re-organized to prepare merging Bucket-Level Alerting into main, the tests/build are expected to fail until we get to the PR that updates the MonitorRunner logic. So I'll be overriding the approval workflow failures as long as the PRs are approved by the reviewers until we've caught up to the end of the dev branch. If at that point, there are any build failures, I'll fix them to pass (although I confirmed the head of the dev branch was stable before beginning to push these PRs).

Can we push to a single branch (bucket-level-alerting-main?) in this repository and not to main branch? That would ensure all the changes for the bucket level alerting goes in a single commit and devs working locally can get those changes once whenever it is merged and not in increments. I am okay either way just wanted to voice my opinion. :)

[qreshi]

Given the way the commits/files were re-organized to prepare merging Bucket-Level Alerting into main, the tests/build are expected to fail until we get to the PR that updates the MonitorRunner logic. So I'll be overriding the approval workflow failures as long as the PRs are approved by the reviewers until we've caught up to the end of the dev branch. If at that point, there are any build failures, I'll fix them to pass (although I confirmed the head of the dev branch was stable before beginning to push these PRs).

Can we push to a single branch (bucket-level-alerting-main?) in this repository and not to main branch? That would ensure all the changes for the bucket level alerting goes in a single commit and devs working locally can get those changes once whenever it is merged and not in increments. I am okay either way just wanted to voice my opinion. :)

Hmm, we don't keep main level branches for features on our repo. So say that we did create that to review each piece currently in the bucket-level-alerting-dev branch. Once each one is merged, we would still need to squash that and make one huge PR into main. At that time, not only would this PR be so large it would be difficult for anyone to look through it reasonably in posterity but we would also need to relate every single PR it was composed of should anyone want to look back at it later. Eventually the bucket-level-alerting branches would be deleted from the repo to keep it clean and then we would lose the incremental history since main only captured it as a single push.

I'm curious what others think though, in case one method is more conventional than the other.

[rishabhmaurya]

Are we using Client anywhere?

[qreshi]

Will update this and the address the comment below in a subsequent PR which has the full implementation of the TriggerService since some things changed between this commit and the final state.

[rishabhmaurya]

this can be a static method.

[dblock]

Did this break the build and still got merged?

[qreshi]

Did this break the build and still got merged?

As is noted above, the way that the files were split to breakup the feature of Bucket-Level Alerting, the tests will fail until we catch up to one of the later changes in which it will be stable again, we aim to be there by today.

[dblock]

@qreshi Bundle build depends on you each repo to at least build without errors, so we can't do that moving forward :( Use branches for similar situations please.

[qreshi]

@dblock Got it, we did have a dev branch to organize the changes in preparation for committing to main but what you're saying is you would have wanted the PRs to happen on that branch (or some other one that is not main) and then once it's reviewed up to a stable point have a big PR to merge it all in?

[adityaj1107]

dB. correct me if I am wrong but I think what you are saying is have a main branch whose build is stable and do whatever you want to do on other dev branches. Have a big PR with links to PRs on the feature-level-main branches etc. etc. or merging each PRs one by one in a short span of time (~1 day) to preserve history is a call which each repo owner can take.