[Commerce] Optimize commerce PR merge

[3ch023]

Hi all,

This is to discuss how/if we can optimize how Milo Commerce PRs are merged.

Problem statement

for commerce, we do have a build and our build artifacts are included in almost every commerce PR.
This means, when PR1 is merged to Stage, even if there is no conflict in /src files, there will be guaranteed conflicts in build artifacts, e.g.:

Idea

Since we are a big team and we want to have more than 1 commerce PR merged in Milo batch, I am thinking about automating the merge of conflicts for build artifacts. This automation only would be triggered if there is no merge conflicts in src file, of course those should be resolved by dev, not script.
Automation scripts would be triggered by a new merge of PR to Stage branch and check if there are conflicts only on commerce artifacts and perform automerge of stage branch in PR branch.

pseudo code

Trigger: merge of PR with lable commerce? will need to check
GH action running on PR:

• checkout PR branch
• check if diff is only in libs/deps/mas/commerce.js, libs/deps/mas/mas.js, libs/features/mas/dist/mas.js
• merge stage branch in PR branch
• cd libs/features/mas
• npm i
• npm run build
• npm run build:docs
• cd ../../../
• git add libs/deps/mas/*
• git add libs/features/mas/dist/mas.js
• set github user etc
• git commit --no-edit
• git push

This is just one of the ideas, pls feel free to share yours! Alternatively, I was thinking about puting build artifacts somewhere else, not git, but didn't find a good way.

[3ch023]

maybe we could only allow this action on PRs with commerce label.
to avoid impacting rest of PRs

[3ch023]

And also I would allow 3-5 file overlapts for all under /libs/features/mas and /libs/deps/mas

[npeltier]

both change sound good to me :) it may also push people adding commerce label ;)

[mokimo]

You might run into a few permission issues, especially if you have PRs raised from forks. There might be ways around that, but you'd have a workflow change both branches in adobecom & forks, which I doubt is easily possible for security reasons.

Another idea could be only creating the artifacts on merge to stage, however then you can't easily test changes on branches since the new artifacts don't exist there 🤔

[3ch023]

Option 2

Another idea is to use comulative PRs.
Gathering a few QA-ed changes in one PR.
The issue here is that the GIT history is mixed up, you can't see anymore who/why did this change in history.

But what if we tweak 'merge-to-stage' to merge some special PRs with 'squash' option. @mokimo @overmyheadandbody do you think it could work?
e.g.

1. we would have a 'commerce' branch
2. we would have a PR from 'commerce' branch to 'stage'
3. only verified commerce PRs are merged into 'commerce' branch
4. 'merge-to-stage' workflow would recognize that this PR is from 'commerce' branch and squash instead of merge commit?

[npeltier]

as you mentionned, it mixes up git history, and i think git history is a very valuable thing. We should not allow cumulative PRs

[mokimo]

Yeah I agree, I'd rather you contribute some logic to the merge-to-stage or even better a new workflow that only runs on stage, to auto generate and push the updated MAS dependencies, you can test that on your own fork first and see if you run into any issues in any scenario

That way no one contributing to MAS needs to know about how the dependencies work as well & u'd only contribute the source files and not the built versions.
We might need to generally kill the "file-overlap" rule later, but that's something we can discuss if generating the built-files on stage & committing those via a workflow doesn't work

[3ch023]

hi @npeltier my idea above is about using squash commit option, meaning all history would be saved

[npeltier]

I don't understand. To me squash commit is the history killer :) erasing all commiter names (bad), and mixing all specific changes (worse) into one big commit, from the squash commiter.

[3ch023]

OPTION 3

Just to note here @yesil suggestion, we could move all our sources back to MAS and do a cummulative artifacts release once a week in Milo.
Or make a Milo an evergreen consumer on mas.js (served by mas repo), however that would mean we will no longer benefit from Milo SOTs check on commerce.

[mokimo]

My vote would be on option 1, however directly on the stage branch. Write a small action that runs the build script & pushes a commit to stage and never worry about actually building the files yourself

[3ch023]

if we don't have artifacts on feature branch we would need to have them somewhere else to test PR changes

[yesil]

I'll start a discussion for option 3, in the coming days.