RFC: MPOC / Stage initiative automation & label simplification

[mokimo]

Stage initiative: Automation with GitHub workflows

Foreword

The long-term goal is to completely get rid of the stage initiative and replace the SOT sign offs with e2e tests. Due to a lot of confusion on how the process works, it still makes sense to automate all the MPoC duties.

The automated version of the stage initiative is designed to streamline our development process, reducing manual interventions, confusion with the process and provide a predictable pipeline to get your PRs to production.

Merges done by the process will be posted in #milo-changelog

Redesign of existing milo core GitHub Labels

We use the following labels to manage the workflow:

• high priority: These PRs are merged first into stage. Developers can use this to fast-track a PR in case the pipeline is clogged. The Jira will need the "BLOCKER" status and have the appropriate business priority.
• Ready for Stage: Applied by QE after testing. Workflow then queues the PR to merge it.
• zero-impact: Auto-labeled by scanning through files changed. No QA required. Files in parts of the repo that won't affect production (e.g., .github/workflows, MEP folder, unit tests) are considered zero impact.
• high-impact: Will post a message in #milo-community for visibility

Two notable changes:

• EVERY PR needs verification. needs-verification is there for a redundant label, as no label indicates it needs testing.
• Since every PR is getting tested verified is the same thing as Ready for Stage

These changes also mean, that QEs would apply 'Ready for Stage' instead of developers.
Testing should not start before all checks are passing.
Testing should also not start before 2 approvals are not given.

Conditions to merge PRs from branch->stage:

Pre-conditions: All checks pass, 2 approvals, has the ready for stage label and not in an RCP

• PR will go into stage with two conditions:
  • No stage->main PR yet (it gets merged and stage->main PR gets created by the workflow)
  • No SOT approvals on stage->main yet (current change can just be merged on top of the other PRs)
• High priority PRs get merged first, otherwise, it's the oldest PR with 'ready for stage' that gets merged first.
• Every PR into stage will only contain unique files (no overlaps, to make reverting easier).

Conditions to merge PRs stage->main

• All checks need to pass
• 4 SOTs sign offs provided
• The labels can be applied by a QE or by running an automated test suite. We encourage consumers to write an action to run a set of tests and apply their sign off label automatically.
• Merges to production will happen during India/EU/US friendly working hours, in case of errors anyone can open a revert PR and re-use the same process to take it to stage. (Initial suggestion: 7am UTC to 9PM UTC Monday to Thursday)
  - Request changes if any bugs are found on stage
• Whoever requests changes, is responsible to unclog the pipeline and remove the label after a fix is issued.

Notifications

A Slack message is sent when PRs are merged into stage and main on #milo-changelog

Handling Issues

If there is an issue after merging stage to main:

• Create PR to revert PR or batch.
• Merge into stage.
• Once 4 SOT labels are there, the process will merge to main. (OR in very bad cases, you could manually apply all the labels for the process to merge once the tests run through)

Handling Conflicts

If two PRs modify the same file, either high priority OR the older PR will be merged first. Once stage and main are synchronised, the other PR will go into stage.

Failing after the Ready for Stage label has been applied

• Checks need to pass or PR does not get merged.
• Approvals need to be there or PR does not get merged.
  Automation does not make a difference, so make sure your PR passes all checks (we can still manually intervene, but your PR can easily get overlooked.)

CSO

• Revert stage batch. Go on the merged PRs and click the "revert" button. (Notify folks to re-open their PRs afterwards)
• Create PR to merge into stage with the fix or production-PR-revert
• Get 2 approvals on it (or an admin can insta merge after tests are green)
• Try to get the fix tested, but in a CSO case it's fine to simply apply the 4 SOT labels and let the process or an admin merge into main.

TL:DR (actionable advice to get ur code to prod):

• Devs: open PR, get 2 reviews, all github checks pass, assign QE
• QE: apply "Ready for Stage" label after testing
• Stage->Main: gets merged on India/EU/US work hours once 4 SOT sign offs are provided & checks are passing
• Bug on prod: Open PR to revert PR or batch and restart at "TL:DR; Devs" (In bad CSO cases, you can manually apply the SOT labels to fast-track a merge or ping an admin)

[mokimo]

Apart of simplifying how labels are currently used, this should capture the entire process we discussed and refined for the stage initiative in the last weeks. Here is an example on how an automated stage->main PR would look mokimo#32

If there are no concerns, I'll be opening a first round-1 PR to automate opening the stage PRs.
Once that is successful and works predictable, we can also merge from stage->main automatically.

[overmyheadandbody]

First of all: great job on this, no one is excited to be MPoC, as it's almost a full time job, so automating this is a big resource/time victory and should be more predictable than humans :)

I'm all in favor of trying it out, with a few mentions:

• define that IST/CET/US timeframe for everyone's awareness;
• we may want to refine the process for critical reverts, waiting for 4 teams to approve might not always be ideal;
• iterate over the code responsible for this whole flow, to make it accessible for more people to edit. Switching around some small things like number of required approvals, label definitions, etc. should be straightforward. I don't want you/us solely owning this every time there's a change in the stage process;
• as an idea for the future, maybe we could have some Slack messages around how many PRs are open, how many are ready for stage, if a PR gets assigned the ready for stage label, etc., to make things a bit easier to follow for everyone, not just developers.

Again, awesome job, I really hope we can get this going!

[mokimo]

define that IST/CET/US timeframe for everyone's awareness;

Initially, I'd suggest with 7am UTC to 9PM UTC Monday to Thursday (adding it to the OP)

we may want to refine the process for critical reverts, waiting for 4 teams to approve might not always be ideal;

I don't think we should push anything to production without prior testing, even if something is broken. Nothing speaks against simply applying 2-3 labels yourself though if it's a really bad bug and then it will get auto merged by the process. It just cares that the labels exist, not who applies them.

iterate over the code responsible for this whole flow, to make it accessible for more people to edit. Switching around some small things like number of required approvals, label definitions, etc. should be straightforward. I don't want you/us solely owning this every time there's a change in the stage process;

I'll try to extract the critical things as variables on the top of the action, so that anyone can edit it easily 😁

as an idea for the future, maybe we could have some Slack messages around how many PRs are open, how many are ready for stage, if a PR gets assigned the ready for stage label, etc., to make things a bit easier to follow for everyone, not just developers.

Great idea! We can start with a few simply messages (merges, etc.) and then exapnd it with as much as we see use in it!

[Blainegunn]

no one is excited to be MPoC, as it's almost a full time job

This is a fact!
In all seriousness, I love this and can't wait to see it in action. I have thoughts.

1. I fell like if a PR needs to be reverted from main because it's causing issues the reversion shouldn't need sign-off the SOTs. Just revert to minimize the outage time; assuming the issue is a CSO. Otherwise I guess I don't care.

2. I love the consolidation of labels. Will the "high priority" label be merged w/ the "high impact"? I see "high impact" is equivalent to a Jira "BLOCKER". What about critical issues?

Thank you Okan 🙏

[mokimo]

I fell like if a PR needs to be reverted from main because it's causing issues the reversion shouldn't need sign-off the SOTs. Just revert to minimize the outage time; assuming the issue is a CSO. Otherwise I guess I don't care.

Yup, as stated you can just apply all the SOT labels yourself when the stage PR is open, but still go through stage first. It would be same way we currently handle CSOs.

We can still see if we want to adapt the process to sidetrack PRs directly to main, but I'd love to not complicate the process further for now. We should have repeatable, predictable steps.

If we think this further of consumers applying their SOT labels by a workflow, we'd also want for those processes to run through to avoid introducing a worse CSO by fixing one in a haste!

I love the consolidation of labels. Will the "high priority" label be merged w/ the "high impact"? I see "high impact" is equivalent to a Jira "BLOCKER". What about critical issues?

Oops typo in the OP, edited that. There should only be high priority which is used to get pushed to the front of the queue of mergeable PRs.
Hopefully, we will only have 1-2 PRs at a time anyway, so it shouldn't need this label to begin with.

Atm we don't have enough PRs to merge anyway, so hopefully critical just gets merged with the usual process within a day or two & faster long term with more e2e automation

[sanrai]

Definitely agree to have a process to minimize the outage time for CSOs versus needing sign-offs.

We had a situation in Milo prior where we had a CSO and required sign-offs to get it reverted but couldn't get that sign-off due to lack of availability/PTO during that time.

Our outage was unnecessarily longer than it needed versus if we had the option to just revert we could've stopped it immediately.

[JasonHowellSlavin]

Fantastic work Okan! I think this change will relieve some of the pressure we all feel.

Question about this:

The labels can be applied by a QE or by running an automated test suite. We encourage consumers to write an action to run a set of tests and apply their sign off label automatically.

Do we have a minimum code coverage goal for the automated test suite before it can be run to apply the label? Also, should we start to include QE automation tests as part of our requirements in new features?

I would love to see the same automation for the SOTs, wondering how we can help to get them there.

[mokimo]

My opinion on this: Every consumer can decide their own coverage goals to what they consider "a safe sign-off" for any milo core code. I'd love to see a consumer throwing the first stone and just testing an automated strategy - before the stage initiative we only had a single QE testing and that also produced reasonable results.

It should be a healthy mix of unit tests, nala tests (from every consumer + milo core tests), PR reviews, developer & reviewers testing the PRs, QE individual PR signoff - and the mix of all should make prod bugs rare, apart of some exotic issues.

There are also two initiatives I'm aware of:

• moving nala tests moving closer to repositories driven by @skumar09 and then you can make writing e2e tests mandatory as part of feature development and ship tests in the same PR as a feature
• @TsayAdobe & @Blainegunn are working on fully automating DC tests/SOT sign-offs, with a longer term vision and I don't have a lot of insight into it and it will likely take a longer amount of time

[vhargrave]

Agree with what everyone else is saying . This is great Okan 🙌
When reverting a bad PR I think that we should just make sure the description says it's really ok to revert without approvals. The wording is important so the person reverting knows they shouldn't focus or wait long for SOT approvals to come in.

What's also not clear to me is how zero-impact PRs are treated. Will they be insta merged without a Ready for Stage label if they have two approvals?

Also , to kick off this process. I think we should start with the automatic PR to stage merging, and once that's working well then automate the stage to main, but I'm sure you were also thinking about starting with slower baby steps too. :)

[mokimo]

What's also not clear to me is how zero-impact PRs are treated. Will they be insta merged without a Ready for Stage label if they have two approvals?

For now, we'll still apply the label - in the future they could by-pass stage completely.

And baby steps definitely 👍 thought about it the exact same way as you

[mokimo]

When reverting a bad PR I think that we should just make sure the description says it's really ok to revert without approvals. The wording is important so the person reverting knows they shouldn't focus or wait long for SOT approvals to come in.

I been wondering about side-tracking PRs and taking them to main directly (zero impact, CSO fixes) and then just have a process resyncing main->stage, what do you/the community thinks about that? It would get us back closer to where we want to be.

It would also avoid clashing with changes that are already on stage, that would need to be reverted first etc.

[vhargrave]

I think that's a very good idea, but only for hotfixes. If something is zero impact I still believe it should go through stage, just in case. You never know when someone is going to make a mistake classifying their PR and suddenly something is broken on Prod because it got fast tracked :D ...

[3ch023]

agree with @vhargrave , if everything always goes through the stage we will have less confusion.

[honstar]

Great stuff @mokimo 👍

One remark regarding this:

high priority: These PRs are merged first into stage. Developers can use this to fast-track a PR in case the pipeline is clogged. Equivalent to a Jira "BLOCKER"

I think we need to change the wording slightly so it becomes clear that a high priority PR is indeed justified by referencing a JIRA ticket that is marked as a blocker.

[mokimo]

First PR: #2224

• Merge PRs into stage
• open stage->main PR

Next up after this works:

• Reworking our labels & connected workflows
• merge stage to main automatically.

[3ch023]

Hi, I went over the docu and got a few questions:

Whoever requests changes, is responsible to unclog the pipeline and remove the label after a fix is issued.

I am afraid this added responsibility might prevent people from reporting issues.
e.g. you recently reported an issue on homepage and even fixed it, but not everyone will always be so responsible to report for some other team and to spent time fixing the issue..
imho, it's already good for us if people start using 'request changes' button if they see a problem. finding an issue shouldn't be 'punished' by extra work.
maybe we could still maintain the mpoc person rotation who will take care of resolving the issue?

Handling Issues

Should we make it clear in the docu that in the case of a CSO, devs shouldn't wait for 4 SOTs to approve?
(i saw a few people above having same thoughts :p)
CSO -> prepare a revert PR for the batch on Stage, tests green -> merge in main ASAP.
Maybe a separate section in docu called 'CSO'? It will be easy to find.
Also, we could create a very detailed docu on what's to be done exactly so that in stressful situations one wouldn't need to think too much and just follow the disaster guide.

[mokimo]

I am afraid this added responsibility might prevent people from reporting issues.
e.g. you recently reported an issue on homepage and even fixed it, but not everyone will always be so responsible to report for some other team and to spent time fixing the issue..
imho, it's already good for us if people start using 'request changes' button if they see a problem. finding an issue shouldn't be 'punished' by extra work.
maybe we could still maintain the mpoc person rotation who will take care of resolving the issue?

Requesting changes doesn't mean that person needs to fix it, just to approve after the issue was fixed. Otherwise, the PR can't be merged from a Github workflow perspective 😁
Rather than an explicit MPOC role, how about just relying on the community?

• Ask for help in #milo-dev/#milo-stage for someone to look into it, which works in every timezone
• Ping every dev that has a PR in the batch to re-check their code

---

Added a CSO section to the opening post, also any comments on it, reply on the comment below: https://github.com/orgs/adobecom/discussions/2206#discussioncomment-9296335 (A new thread might be a little more visible//better to add comments on)

[mokimo]

It will likely still need MPOCs for a while, but maintaining a schedule brought a lot of confusion and overhead with it so far. I'd like to completely kill that role in the near future

In a CSO case everyone is liable and needs to jump in.
In a broken stage case, I'm still sure there would be volunteers from the community to help getting things fixed

[3ch023]

and yes, huge kudos on this work!

[mokimo]

CSO procedure, wdyt?

CSO

• Revert stage batch. Go on the merged PRs and click the "revert" button. (Notify folks to re-open their PRs afterwards)
• Create PR to merge into stage with the fix or production-PR-revert
• Get 2 approvals on it (or an admin can insta merge after tests are green)
• Try to get the fix tested, but in a CSO case it's fine to simply apply the 4 SOT labels and merge into main.

[mokimo]

Progess done
Workflow-1: shipped branch->stage workflow:
Merge PRs into stage and open the sync PR

• https://github.com/adobecom/milo/blob/stage/.github/workflows/merge-to-stage.yaml
• https://github.com/adobecom/milo/blob/stage/.github/workflows/merge-to-stage.js

Progress todo
Workflow-2: merge stage into main
Workflow-3?: send all high impact PRs into a special channel
Workflow-4: auto label zero impact PRs (and remove the labels where not appropriate) - those could then be merged into stage even after sign offs.

[narcis-radu]

@mokimo - We should have a deep dive session on labels and workflows to make sure everyone understands the changes and how to apply the correct values.

[mokimo]

Great idea, this goes well in line with the label simplification PR I just opened #2288 which slightly adapts on how the labeling process currently works.

With #2290 the bulk part of the stage automation will be done, and I can host a deep-dive about how the automation works in detail. Should make up for a fun session

[mokimo]

PRs with failing checks aren't merged automatically, would be great to brainstorm on how we can fix this and ensure the check is ALWAYS green...
PR 2294
Failing codecov check. 4k lines of code covered, 70% coverage instead of 100%. This one PR brings the project coverage down by 0.5%.
Merge or force to write more tests?

[overmyheadandbody]

We should ask for better coverage. Depending on their commitments, if they can't do it right now we should ask them to open a critical story on their end to get the code coverage to a good point asap.

[narcis-radu]

I agree with @overmyheadandbody - we should improve coverage on the code. If it turns out to be something critical, let's at least have a follow up ticket to improve coverage.

[mokimo]

PRs with failing checks aren't merged automatically, would be great to brainstorm on how we can fix this and ensure the check is ALWAYS green...
PR 2279
Failing PSI check. However, PSI fails on a tooling page (caas) - what do we want to do in these cases?

[sanrai]

That PR is somewhat of an exception since it's for a CaaS authoring tool and running LH checks on that doesn't really make sense.

So in the past, we've ignored that PSI checks and merged since we knew it had no impact on customers.

[overmyheadandbody]

Agree, caas could be treated as an exception, as it's not customer-facing

[narcis-radu]

Is there a way to automatically skip PSI checks if the URL is **/tools/** ?

[mokimo]

Not that I am aware of. As workaround just to get the check green, I've moved the test URLs and used dummy ones. I'm not convinced this is a great option though.
It's a lot better than programmatically trying to skip PSI checks, unless easily supported by the aem bot.

[mokimo]

We could write some logic to only consider PSI checks a failure if they are on certain paths, but from a maintenance perspective this sounds awful.

[mokimo]

PRs with failing checks aren't merged automatically, would be great to brainstorm on how we can fix this and ensure the check is ALWAYS green...
PR 2276
Failing nala check. IMO failing nala = no merge... anyone any other ideas/input on this?

[mokimo]

@skumar09 any opinion on this with nala?

[overmyheadandbody]

This is debatable. The issue seems to be with the Marketo forms. I did a manual run on Stage, which in theory should not have issues. But it does, screenshot below

So I don't think we should tax innocent committers for issues that are already there. We should just ask @skumar09's help to get these fixed.

[mokimo]

PRs with failing checks aren't merged automatically, would be great to brainstorm on how we can fix this and ensure the check is ALWAYS green...
PR 2244
Failing codecov.

No unit tests exist for the caas bulk publisher. Fixing GH check would require writing unit tests for entire file which is out of scope for this ticket.

Do we want to allow to ignore certain files/folders? https://docs.codecov.com/docs/ignoring-paths
As authoring tools such as the one for CaaS are purely internal, we still shouldn't have any bugs/CSOs on that, but it would be less severe.
Wdyt?

[sanrai]

Yeah your point there pretty much encapsulates what I was trying to say.

From a PR perspective, how should one go about ignoring paths? Would we add that somewhere in the PR description and it gets parsed and excluded by the corresponding GH action?

[sanrai]

Keeping slack discussion reference to above discussion here:

Recommendation was to add paths to be excluded in codecov.yaml file

For codecov, i'd add the file to be excluded in the codecov.yaml

https://adobe-mwp.slack.com/archives/C06CX6EUNBG/p1715669774824109?thread_ts=1715668154.321209&cid=C06CX6EUNBG

[mokimo]

I'm not a fan of labels at all, but I wonder if we should just have a catch-it-all solution such as a label to merge PRs despite failing checks Force Ready for Stage ... any opinions for / against this?

There are overall valid reasons for a PR to get merged.

• File is not covered and out of scope for a PR to start tests
• Failing PSI checks on certain pages/scenarios (MEP, CaaS, non consumer facing pages)
• Nala checks for a specific string/class that changes & there is an open/pending nala PR to be merged after the milo PR gets merged
• etc.

This would provide developers the freedom to get their code into stage despite failing checks, in a self-serve manner.

[overmyheadandbody]

I fear this could become a golden ticket. I know you'd rather have everything automated, but I think it would be better to have some predefined exceptions to the rules in your merge script, if possible. Otherwise manual input might be needed.

[mokimo]

So the current exceptions I'd see in that case would be:

• ignoring PSI (workaround, testable link in comments, dummy link in description)
• ignoring nala (workaround, remove the label so the tests aren't running)
• ignoring codecov (workaround, see here: https://github.com/orgs/adobecom/discussions/2206#discussioncomment-9429454 )

In specific cases, it can't be expected to have those checks green all the time. 🤔

I wonder if we should attempt workarounds, just ignore those checks from the stage->main perspective - or come up with any other ideas

[narcis-radu]

A catch-it all label sounds like over-engineering at this point and it might get even used wrongfully. If there are known limitations (e.g. PSI irrelevant for non customer facing pages), we can work around it and have better control on which PRs get merged.

[robert-bogos]

I also agree that we should have more control over these scenarios. A catch-it-all label can indeed be exploited by people

[sanrai]

I'd recommend starting out not whitelisting everything and just having labels be more specific.

E.g. A CaaS Authoring Update label would cover most of our PR to Milo (since we mainly just contribute updates to authoring tools)

Furthermore, if we have changes that can affect customers/end users, we can remove that label and still leverage the built in PSI checks/Unit test coverage/etc. in Milo without causing you guys too much overhead in the merging process.

I'm not a fan of labels at all, but I wonder if we should just have a catch-it-all solution such as a label to merge PRs despite failing checks Force Ready for Stage ... any opinions for / against this?

[robert-bogos]

Great work! 👍
As Narcis said, we'll definitely need a deep dive session to make sure that people understand the changes made.