feat: refactor mergify APIs #1766
Conversation
Signed-off-by: github-actions <github-actions@github.com>
Codecov Report
@@ Coverage Diff @@
## main #1766 +/- ##
==========================================
+ Coverage 88.06% 89.95% +1.89%
==========================================
Files 132 153 +21
Lines 5109 6234 +1125
Branches 1207 1597 +390
==========================================
+ Hits 4499 5608 +1109
- Misses 610 621 +11
- Partials 0 5 +5
Continue to review full report at Codecov.
|
|
@Mergifyio rebase |
|
@pgollucci is not allowed to run commands |
| @@ -40,6 +40,19 @@ const project = new javascript.NodeProject({ | |||
| }); | |||
| ``` | |||
|
|
|||
| ## Automatic Merging | |||
There was a problem hiding this comment.
I feel like this doc is missing a code snippet or two. I feel like there should be an example for configuring automatic merging as well as what to do if you want to configure Mergify in your project for something else.
|
|
||
| /** | ||
| * Configure options for automatic merging pull requests on GitHub. Has no | ||
| * effect if `autoMerge` is set to false or `github` is set to false. |
There was a problem hiding this comment.
I'm not too familiar with projen customs, but should we simply error out if autoMergeOptions is set and autoMerge or github is set to false? Seems odd to just ignore this property. Same goes for autoMerge
| * | ||
| * @default true | ||
| */ | ||
| readonly mergify?: boolean; |
There was a problem hiding this comment.
Since this is a fairly big API transition and we're removing a few properties (esp the non-deprecated ones like these), I feel like we need to have strong migration docs between projen versions for users who are broken.
I think I understand the vision of projen where we have lots of major versions and you don't really need to upgrade projen in your project. But if I'm a die-hard projen user with some ideas on how to set up mergify in an old project (in projen v0.34 for example) and it now doesn't exist when I'm trying to set up a new project (in projen v0.59 or something), I'm going to want to have some docs that tell me what to do now. I don't really know if there's a right way to achieve this though.
There was a problem hiding this comment.
I guess the BREAKING CHANGE comments go into the changelog? but do people actually rely on the changelog for help?
There was a problem hiding this comment.
I think putting all breaking changes / migration suggestions in the changelog is probably okay since it's a single source of truth for high-level API changes and it's easy to ctrl+f for some API name. But I agree it's a good idea to link to detailed docs as well. It could also be nice if the docs of any component listed the latest changes / versions of projen it was changed in etc.
We don't generate a single changelog file anywhere right now, so maybe that should be addressed first...
| approvedReviews: 2, | ||
| blockingLabels: ["draft"], | ||
| }); | ||
| autoMerge.mergify.addRule({ |
There was a problem hiding this comment.
is exposing mergify a leaky abstraction? especially if we plan on providing other auto-merging backends in the future (per the docs you wrote).
There was a problem hiding this comment.
Hmmmm, yeah you're totally right.
In that case I'm not sure how much the API in this PR makes sense anymore. 😅 Maybe I'll put this back on hold for now
The motivation for this PR is to simplify the APIs for
MergifyandAutoMerge. Mergify configuration is currently getting configured from multiple places (bothmergifyOptionsandautoMergeOptions), which leads to confusion since it's likely many users will only discover one or the other.In another sense, projen's API is exposing options both through the L2
Mergify(an intent based API for managing Mergify configuration files) and the L3AutoMerge(and intent based API for managing the concept of auto-merging, which relies onMergifyas an implementation detail). I think this makes the design more complicated / confusing to users.Instead this PR changes the mental model so we just expose the L3 API (
autoMergeandautoMergeOptions) in base project options, so mergify has to be either configured throughAutoMerge, or created and managed separately.BREAKING CHANGE:
mergifyandmergifyOptionsare no longer available to directly configure at the project level. Instead, useautoMergeandautoMergeOptions. If you would like to disable automerging, addautoMerge: falseto your project constructor and create a newMergifycomponent directly.Githuborproject.githubanymore, instead it is a property ofAutoMergeorproject.autoMerge.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.