Milo Stage Process

[3ch023]

New Process Wiki: Milo Stage Initiative

Today is the first day and I have still some amount of open questions.
I suggest we use this discussion for any question/suggestion/action item around the process.

Questions

• How does MPoC (milo person of contact, the one who merges pr's) reach out to T1 approvers?

ToDo

1. suggest&inform Devs on 'Ready for Stage' label
2. document labels (if missing)
3. adapt branch protection rules, see Details
4. automate the creation of PR into main -> in progress, see PR
   5. Gather all T1 consumers and bundle them up in a slack-channel so MPoCs can reach out for testing. Note we have SOT Teams & the first open question in regard to communication
5. Inform Devs about the necessity of 'critical' and 'blocker' labels!!!

[overmyheadandbody]

Hey @3ch023! Good luck on this first week, it will probably be a bumpy one :)

Regarding your question: @Blainegunn mentioned in the meeting last week that he'd set up some Slack channels - one for T1 consumers, one for MPoCs to sync when switching shifts. Are those available (I didn't see any communication around that)? If not, it's something that should be implemented quite soon.

Regarding your suggestion: 100% agree, if a commiter doesn't bother to add all the required labels, the PR should not be seen as important/critical, nor should the MPoC go through all the details to judge priority; that should be instantly clear from the PR itself.

[3ch023]

I see one for MPoC but not the T1 consumers.

Also, the auto PR creation didn't happen, so I am going to do one manually

After the PR is merged into the stage branch a new PR will automatically be raised against main.

[Blainegunn]

Slack Channels
#milo-point-of-contact - Space for MPoCs keep each other updated
#milo-sots - Space for MPoC to get PR approvals for main branch from T1/SOTs
#milo-stage - Space for all other topics

[mokimo]

Should we add a READY FOR STAGE label?

A lot of PRs even with passing checks, might still have some feedback or pending changes until they are fully ready. A developer should be responsible to indicate the pr is "done" to pass it off to MPoCs to merge.

MPoCs should be able to filter by the "READY FOR STAGE" and then given secondary labels (critical/blocker/trivial) or time elapsed since a PR has been opened decide which PR to merge

[3ch023]

'Ready for Stage' sounds perfect to me

[meganthecoder]

My PR is ready and I'd like to label it correctly. I see someone using a "Ready to Merge" label. Is that the same or should we create a new "Ready for Stage" label as you suggested?

[yesil]

Regarding the verified label, do we still use it or rely solely on Ready for stage label.
OR QA adds verified, and Dev adds Ready for stage?

[3ch023]

My PR is ready and I'd like to label it correctly. I see someone using a "Ready to Merge" label. Is that the same or should we create a new "Ready for Stage" label as you suggested?

as of now we are a bit in chaos, so i was checking also not labeled PRs, but in green&approved&verified state.
tomorrow in the office hours I suggest we decide something concrete together and spread the word

[npeltier]

Regarding the verified label, do we still use it or rely solely on Ready for stage label. OR QA adds verified, and Dev adds Ready for stage?

i would say both labels still have some sense to me some thing could be verified by QA, but not ready for stage (some dev feedback or whatever)

[mokimo]

Should we adapt the branch protection rules?

• Currently merging to stage does not require any reviews. It should also match the rules we have set up for main, only PRs with 2 reviews should be mergeable.
• Merging to main currently only requires 2 reviews, strictly following the process set up by @Blainegunn those would be minimum 5 reviews now (A 👍 from every T1 consumer)

[3ch023]

we should, yes
will add that to description of the topic.
btw do you have permission to edit main description text? if yes, please feel free to edit in future. if not, i will need to update it, but its ok

[mokimo]

Yup, got permission. Let's discuss this on wednesday and then implement it, if there is no opposition

[3ch023]

cool, for now i added, see todo -> item 3

[3ch023]

another thing I thought about looking at our guinea pig pr #1954
there are few people for each SOT, so PR ends up having 10+ reviewers.
MPoC needs to quickly understand if project X approved the PR or not. Its hard to check name by name which project the person is approved for.

we need to think how to make it more clear.
either label or comment maybe? 'approved for HomePage and DC', 'Approved for CC'. I think I prefer comment since its more flexible.

[3ch023]

it was actually my mistake, yes, it should be teams instead of individuals.
sorry for the confusion!

[overmyheadandbody]

Yes, if we ever want to have a more automated process, labels are probably the way to go, as they should be standardised as opposed to comments. Also, it can become difficult to scroll through multiple comments, whereas labels should be visible in the first screen - actually even without accessing the PR, since the list of open PRs also lists labels directly.

However, we already have a bunch of labels, we should think of ways to better organize them. We have a few categories of labels - process ones (trivial, needs-verification, etc.), notification ones (breaking change, new feature), NALA ones, ready for X (stage/main). Maybe we come up with some prefixes for each category so people know what to choose from the ever-growing list.

[mokimo]

Created a ticket for the labels in Eurovisions name, we'll come up with an initial draft and then raise this as a discussion with the community for specific input.

[narcis-radu]

One issue with the sot is - <user> approved these changes on behalf of adobecom/admins, adobecom/homepage-sot, adobecom/document-cloud-sot, adobecom/creative-cloud-sot, adobecom/miq-sot, and 1 other team 9 minutes ago.

When an user is part of multiple groups / teams, it can approve for all of them, defeating the purpose of having multiple teams to sign off.

[3ch023]

if we have the label per consumer as discussed above instead of a regular PR approval, the issue will be solved @narcis-radu
wdyt?

'CC approved'
'DC approved'
...

[mokimo]

Should we allow PR feedback such as code quality or missing unit tests on the stage->main sync?

Personally, I would go with a definite no. Unless a critical bug is spotted, either from testing OR from sanity checking the code - that would be fine to block the merge.

Everything else such as code style improvements // missing unit tests // non-problematic feedback need to be addressed in the initial PR OR suggested as follow-ups (in the initially merged PR)
You can still leave comments on merged PRs and the PR initiator should be notified by Github or you can raise a ticket in the PR-initiators name to follow up with improvements on a new PR.

Blocking stage->main PRs for improvements on an already inefficient pipeline, sounds like a very bad idea to me

[3ch023]

agree.
good topic to mention in the next call :)

stage->main PR is considered reviewed, no need for a second code review.
unless a critical bug is spotted, as you mentioned.

[overmyheadandbody]

100% agree. The time for code reviews is before Stage. Once a PR is pushed to Stage, I'd see the following scenarios:

• no further concerns -> merge to main
• minor concerns, such as unit tests, formatting, a slightly better approach -> merge the PR to main, open follow-up story for the committer
• major concerns -> rolled back from stage

[fullcolorcoder]

I agree @mokimo and thanks for carrying this conversation.