refactor!: move fork down to GitHub #770
Conversation
fork is a low level GitHub plumbing mechanism. GitHub can make the decision when it will be able or unable to add/remove labels and close prs. BREAKING CHANGE: fork is no longer in ReleasePRConstructorOptions, it is now in GitHubConstructorOptions.
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #770 +/- ##
==========================================
- Coverage 90.56% 89.85% -0.71%
==========================================
Files 60 60
Lines 7850 7855 +5
Branches 763 587 -176
==========================================
- Hits 7109 7058 -51
- Misses 738 786 +48
- Partials 3 11 +8
Continue to review full report at Codecov.
|
| }) | ||
| .option('fork', { | ||
| describe: 'should the PR be created from a fork', | ||
| type: 'boolean', |
There was a problem hiding this comment.
Reading through factory.ts, It didn't look like fork is passed to githubInstance in the releasePR helper?
Does it still need to be set here so that the branch can be created from a fork?
There was a problem hiding this comment.
good catch, that means I'm missing a test! will fix (and I think there's an even simpler way to pass around Factory config options to the different factory helpers)
bcoe
left a comment
bcoe
left a comment
There was a problem hiding this comment.
This is looking good 👍
It might be good to open a work in progress PR on release-please-action that captures some of these additional breaking changes, such as switching from run to call.\
We can keep publishing candidate releases of release-please as we continue refactoring work.
DRY'd out factory.gitHubInstance options gathering with getGitHubFactoryOpts so that factory.releasePR and factory.githubRelease both get a consistently configured GitHub instance. Added test that all the same options passed to factory.githubInstance and to factory.releasePR produce the same GitHub instance. Also simplified the `releaseType` factory config to be optional for both factory.releasePR and factory.githubRelease: now relying on the cli to default to 'Node' for `release-pr`. Anyone else who accidentally ends up with a ReleasePR instance when they wanted an actual releaser to run will find out soon enough as `run()` will go boom.
| // ReleasePR and GitHubRelease Factory | ||
| interface ReleaserFactory { | ||
| releaseType?: ReleaseType; | ||
| } | ||
|
|
||
| // ReleasePR factory/builder options | ||
| // `releaseType` is required for the ReleaserPR factory. Using a type alias | ||
| // here because the `interface ... extends` syntax produces the following error: | ||
| // "An interface can only extend an identifier/qualified-name with optional | ||
| // type arguments." | ||
| export type ReleasePRFactoryOptions = ReleasePROptions & | ||
| GitHubFactoryOptions & {releaseType: ReleaseType; label?: string}; | ||
| export interface ReleasePRFactoryOptions | ||
| extends ReleasePROptions, | ||
| GitHubFactoryOptions, | ||
| ReleaserFactory { | ||
| label?: string; | ||
| } | ||
|
|
||
| // GitHubRelease factory/builder options | ||
| export interface GitHubReleaseFactoryOptions | ||
| extends GitHubReleaseOptions, | ||
| ReleasePROptions, | ||
| Omit<ReleasePRFactoryOptions, 'releaseType'>, | ||
| ReleasePRFactoryOptions, |
There was a problem hiding this comment.
decided maintaining a required vs optional releaseType depending on the factory was too complicated - I think we can rely on the cli defaulting to 'Node' for release-pr. Anyone else who accidentally ends up with a ReleasePR instance when they wanted an actual releaser to run will find out soon enough as run() or getOpenPROptions will go boom.
| if (isReleasePRCmd(cmdOpts)) { | ||
| if (isGitHubReleaseCmd(cmdOpts)) { | ||
| result = factory.call(githubRelease(cmdOpts.options), 'run'); | ||
| } else if (isReleasePRCmd(cmdOpts)) { | ||
| let method: ReleasePRMethod = 'run'; | ||
| if (command === 'latest-tag') { | ||
| method = 'latestTag'; | ||
| } | ||
| result = factory.call(releasePR(cmdOpts.options), method); | ||
| } else if (isGitHubReleaseCmd({command, options})) { | ||
| result = factory.call(githubRelease(cmdOpts.options), 'run'); | ||
| } else { | ||
| throw new Error( | ||
| `Invalid command(${cmdOpts.command}) with options(${JSON.stringify( | ||
| cmdOpts.options | ||
| )})` | ||
| `Invalid command(${command}) with options(${JSON.stringify(options)})` |
There was a problem hiding this comment.
I'm not a huge fan of this complex type checking code - when I made releaseType optional on both {GitHubRelease,ReleasePR}FactoryOptions it introduced a type error in the original code here saying cmdOpts had "acquired the never type" after the isReleasePRCmd type predicate/guard check. I'm not exactly sure why, perhaps because now ReleasePRFactoryOptions is a subset of GitHubReleaseFactoryOptions (it used to have a required releaseType property). It hurts my brain a little but it works switching the if/else-if conditionals around
| const releasePR = factory.releasePR({ | ||
| repoUrl: 'googleapis/ruby-test-repo', | ||
| fork: true, | ||
| token: 'some-token', |
There was a problem hiding this comment.
I like this smoke test 👍 I should probably do something similar in the action.
2 participants
fork is a low level GitHub plumbing configuration. GitHub can make the
decision when it will be able or unable to add/remove labels and close
prs.
BREAKING CHANGE: fork is no longer in ReleasePRConstructorOptions, it is
now in GitHubConstructorOptions.