I like the intent here, but I think some things should be clarified.
If the swagger files are being autogenerated, then why would you ever review them? You should test them, but reviewing isn't appropriate. You should review the things that generate them instead. When they change, that's a build artifact (I don't review my generated libraries and binaries, I only test them before delivering them).
On the commitbot, I hope you're not doing that in deployment without running it through test first. Can it also generate issues if a particular service fails its tests after the update?
re: why review; I commented about this above, but:
1. We prefer to be able to say that everything is reviewed, but that in this particular case the review works automatically. Requiring a PR review is important to our security model (and therefore, our business). Skipping review, especially in a library that contains both these definitions and the code that leverages them would be too cavalier for us.
2. We want to give developers control over when these changes are merged. Many API changes are coordinated with other changes (e.g. in integrating services). We _could_ require backwards compatibility, but we find this too onerous in this layer of our stack.
re: test first; we have a very thorough process for qualifying releases:
- As much as we like continuous deployment, our (enterprise) customers aren't particularly interested in continuous changes to their experience and the $VALUE of a customer to us is far to high to take that kind of risk.
- Practically speaking, we adopt the git-flow model and have separate deployment environments for integration (`develop` branch), QA (`release/` branch), staging (`tags/`) and then promote changes out to production, subject to suitable approvals/controls. I'm fully aware that this approach may sound cumbersome, but it's not in our line of business.
Comments
I like the intent here, but I think some things should be clarified.
If the swagger files are being autogenerated, then why would you ever review them? You should test them, but reviewing isn't appropriate. You should review the things that generate them instead. When they change, that's a build artifact (I don't review my generated libraries and binaries, I only test them before delivering them).
On the commitbot, I hope you're not doing that in deployment without running it through test first. Can it also generate issues if a particular service fails its tests after the update?
re: why review; I commented about this above, but:
1. We prefer to be able to say that everything is reviewed, but that in this particular case the review works automatically. Requiring a PR review is important to our security model (and therefore, our business). Skipping review, especially in a library that contains both these definitions and the code that leverages them would be too cavalier for us. 2. We want to give developers control over when these changes are merged. Many API changes are coordinated with other changes (e.g. in integrating services). We _could_ require backwards compatibility, but we find this too onerous in this layer of our stack.
re: test first; we have a very thorough process for qualifying releases:
- As much as we like continuous deployment, our (enterprise) customers aren't particularly interested in continuous changes to their experience and the $VALUE of a customer to us is far to high to take that kind of risk. - Practically speaking, we adopt the git-flow model and have separate deployment environments for integration (`develop` branch), QA (`release/` branch), staging (`tags/`) and then promote changes out to production, subject to suitable approvals/controls. I'm fully aware that this approach may sound cumbersome, but it's not in our line of business.
Great question
commitbot and mergebot both go through CI first and are linked to the github health checks.
Mergebot will not merge a PR if it fails the tests, commitbot only generates the PR (which goes through tests like any other commit).