Apply patches

[nat-henderson]

This solves a weird problem that I think might start coming up more often.

• You create a MM PR (call it MMPR1), and it creates two PRs downstream, in Terraform and Puppet, call them TFPR1 and PPR1.
• TFPR1 is merged, but PPR1 is not.
• Consequently, MMPR1 cannot be merged.
• You create MMPR2, which affects Terraform, and creates TFPR2.
• MMPR2 does not include the commit of MMPR1, which is not merged.
• TFPR2 reverts the changes of TFPR1, because the code used to generate it is not present in the version of MagicModules that MMPR2 is based on.
• Worse, MMPR3, which makes no changes to the downstream code, will open TFPR3, reverting TFPR1.

This solves the problem in a way that I kind of like, but I want your opinion on - we fetch any PR which is merged downstream, as a diff which should be applied after our changes. If that PR's patch applies cleanly, that's great. If it doesn't, then we're in a state where we actually can't generate a correct PR, and generation should fail (as it does, in this change) rather than produce a PR which will apply incorrectly.

NB: This has the maybe somewhat forward-looking benefit that it is agnostic to the source of the patches. If someone decided they just had to make a modification to a Terraform resource, but didn't want to bother with the MagicModules work to make that change, this could be modified to fetch that PR as well. Might be handy to have.

Another note - The order of operations here is

• Generate normally & commit
• Apply patches
• Reset git state to before the first commit
• Commit again

This is because applying patches fails if your working tree isn't clean. We need the patches to apply after the generation, because otherwise generation will blow those changes away again. We want the whole thing (generation + patches) as a single commit because splitting it up won't provide any useful information.

What do you think?

---

[all]

CI change only

[terraform]

[puppet]

[puppet-dns]

[puppet-sql]

[puppet-compute]

[chef]

[rosbo]

I like that change and this is something we should do.

One side note, using your scenario with MMPR1 and MMPR2.

Before this change, TFPR2 would be garbage (because it would try to revert TFPR1)
After this change, if TFPR1 doesn't apply cleanly on top of TFPR2. Then you are stuck until MMPR1 is merged.

This change solves the problem when TFPR1 applies cleanly (which should hopefully be most of the times) but we are still stuck for the case when it doesn't apply cleanly. I don't think there is an easy way to solve this.

In order to reduce the likelihood of being stuck in that situation, should we all agree that if we have a MM PR that impacts broadly (likely to cause merge conflicts) many downstream PRs, then, we should wait until all the downstream PRs are ready to merge before merging them. Thoughts?

[nat-henderson]

Yep, I think that if you end up in a situation where TFPR1 doesn't apply cleanly on top of TFPR2, then it is unavoidable that you're stuck until MMPR1 is merged. I imagine you might solve this with a git rebase gcp/mmpr1-branch, but that's sufficiently likely to have bad side effects that I don't want the magician to do it (at first glance).

I agree with you - I think a broad impact PR shouldn't be merged until all its downstreams are ready - but I think there will probably be cases where we do it anyway, for instance resources that people are particularly interested in.

[modular-magician]

I am a robot that works on MagicModules PRs!
I checked the downstream repositories (see README.md for which ones I can write to), and none of them seem to have any changes.

Once this PR is approved, you can feel free to merge it without taking any further steps.