refactor(manifest): manifest handling cleanup #805
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #805 +/- ##
==========================================
+ Coverage 92.83% 92.92% +0.09%
==========================================
Files 63 63
Lines 8573 8683 +110
Branches 833 897 +64
==========================================
+ Hits 7959 8069 +110
Misses 611 611
Partials 3 3
Continue to review full report at Codecov.
|
joeldodge79
force-pushed
the
manifest2-switch-to-mocks
branch
from
af2b22c
to
a3f7c63
Compare
| protected async getFile(fileName: string): Promise<GitHubFileContents>; | ||
| protected async getFile( | ||
| fileName: string, | ||
| sha: string | ||
| ): Promise<GitHubFileContents | undefined>; |
There was a problem hiding this comment.
I may be abusing overloading here - this getFile function might be trying to do too many different things
getFile('release-please-config.json')- get the config filegetFile('.release-please-manifest.json', 'abc123')- get the manifest at last release shagetFile('.release-please-manifest.json')- get the manifest at tip of default branch
Instead of breaking it apart I've tried to clarify the different usage with overloads but that might just be silly - I'm still on the fence - happy to receive a nudge either way (keep overloads or break up function into separate helpers)
There was a problem hiding this comment.
I might just be tempted to add an additional getManifest helper which calls getFile, but understands the sha parameter.
There was a problem hiding this comment.
getManifest is the helper :-)
(overloaded to distinguish between a call for latest manifest version at tip of default branch (no sha) vs manifest from last release (sha required))
| } | ||
| const releaserOptions = { | ||
| monorepoTags: packages.length > 1, |
There was a problem hiding this comment.
I originally thought this could make the manifest releaser behave exactly like just using a regular releaser on a single package repo (i.e. no need for monorepoTags). But I decided that added too much complexity. So, even though the manifest releaser can be configured on a single package repo, it will still produce the packageName.getComponent() in the tag/release name
There was a problem hiding this comment.
That seems like the right call to me 100%. I'd honestly like us to move to the stable branch name across the board, because it would make it easier to back port to LTS branches.
| } | ||
| } | ||
| return Object.values(packagesToRelease); | ||
| } | ||
|
|
||
| private async validateJsonFile<T extends object>( |
There was a problem hiding this comment.
now that the manifest file type is VersionsMap it was unwieldy to try to keep this generic. type coercion pushed back into callers
and anyway, hopefully this will get swapped out for ajv soonish...
| @@ -186,7 +186,7 @@ export class ReleasePR { | |||
| } | |||
| } | |||
|
|
|||
| protected defaultInitialVersion(): string { | |||
| defaultInitialVersion(): string { | |||
There was a problem hiding this comment.
I log this during the Manifest.pullRequest run so I need it to be public
| .withExactArgs('.release-please-manifest.json', lastReleaseSha, false) | ||
| .rejects(Object.assign(Error('not found'), {status: 404})); | ||
| } else { | ||
| mock.expects('getFileContentsWithSimpleAPI').never(); |
There was a problem hiding this comment.
really appreciating the strictness that mock allows - all though I do wonder if I'm testing the implementation details too closely...
| [ | ||
| 'Found version 3.2.1 for node/pkg1 in ' + | ||
| '.release-please-manifest.json at abc123 of main', | ||
| CheckpointType.Success, | ||
| ], | ||
| [ | ||
| 'Failed to find version for node/pkg2 in ' + | ||
| '.release-please-manifest.json at abc123 of main', | ||
| CheckpointType.Failure, | ||
| ], | ||
| [ | ||
| 'Found version 1.2.3 for python in ' + | ||
| '.release-please-manifest.json at abc123 of main', | ||
| CheckpointType.Success, | ||
| ], | ||
| [ | ||
| 'Bootstrapping from .release-please-manifest.json at tip of main ' + | ||
| 'for missing paths [node/pkg2]', | ||
| CheckpointType.Failure, | ||
| ], | ||
| [ | ||
| 'Failed to find version for node/pkg2 in ' + | ||
| '.release-please-manifest.json at tip of main', | ||
| CheckpointType.Failure, | ||
| ], | ||
| ['Processing package: Node(@node/pkg1)', CheckpointType.Success], | ||
| ['Processing package: Node(@node/pkg2)', CheckpointType.Success], | ||
| [ | ||
| 'Falling back to default version for Node(@node/pkg2): 1.0.0', | ||
| CheckpointType.Failure, | ||
| ], | ||
| ['Processing package: Python(foolib)', CheckpointType.Success], |
There was a problem hiding this comment.
I feel like this kind of output will be useful to end-users trying to debug why a package is getting the version it is
- stronger typing for manifest file: using VersionsMap type
- cleanup logic around manifest handling:
1. retrieving manifest - use overload to distinguish between a call
for the manifest at a specific sha vs at HEAD
2. clearer logic to handle bootstrapping (i.e. falling back to HEAD
manifest and then to defaultInitialVersion)
3. more logging around version determination.
- switch to using a mock in tests
- typo fix in lastMergedPRByHeadBranch graphql query
joeldodge79
force-pushed
the
manifest2-switch-to-mocks
branch
from
a3f7c63
to
bda21fe
Compare
for the manifest at a specific sha vs at HEAD
manifest and then to defaultInitialVersion)