feat(symbol-collector): Add symbol-collector target#266
Merged
Conversation
Instead of requiring having it installed locally, download the symbol collector and use the binary.
iker-barriocanal
commented
Jul 9, 2021
AbhiPrasad
reviewed
Jul 9, 2021
These were used in the past, but are no longer required.
bruno-garcia
reviewed
Jul 9, 2021
Member
bruno-garcia
left a comment
There was a problem hiding this comment.
sweet! Left a couple suggestions. Thanks @iker-barriocanal
BYK
approved these changes
Jul 9, 2021
Member
BYK
left a comment
There was a problem hiding this comment.
Didn't look too deep into tests but overall LGTM, great job!
AbhiPrasad
approved these changes
Jul 12, 2021
Contributor
AbhiPrasad
left a comment
There was a problem hiding this comment.
Nice! I would wait for Burak's answer about unzip before merging, but otherwise looks solid
BYK
reviewed
Jul 27, 2021
| artifactProvider: BaseArtifactProvider | ||
| ) { | ||
| super(config, artifactProvider); | ||
| this.symbolCollectorConfig = this.getSymbolCollectorConfig(); |
Member
There was a problem hiding this comment.
This seems like a common pattern so maybe we should abstract config parsing into the base class and make sure it is always called by the default constructor parseTargetSpecificConfig()
Comment on lines
+73
to
+78
| if (artifacts.length === 0) { | ||
| this.logger.warn(`Didn't found any artifacts after filtering`); | ||
| return; | ||
| } | ||
|
|
||
| this.logger.debug(`Found ${artifacts.length} symbol artifacts.`); |
Member
There was a problem hiding this comment.
Should this logic be part of the base target class?
| artifacts.map(async (artifact, index) => { | ||
| const subdirPath = join(dir, String(index)); | ||
| await fsPromises.mkdir(subdirPath); | ||
| await this.artifactProvider.downloadArtifact(artifact, subdirPath); |
Member
There was a problem hiding this comment.
Might be better to just return the final promise:
return this.artifactProvider.downloadArtifact(artifact, subdirPath);|
|
||
| jest.mock('../../utils/files'); | ||
| jest.mock('../../utils/system'); | ||
| jest.mock('fs', () => { |
Member
There was a problem hiding this comment.
We may wanna lift the common fs mocks into a global mock module as this pattern seems common enough now.
| (checkExecutableIsPresent as jest.MockedFunction< | ||
| typeof checkExecutableIsPresent | ||
| >).mockImplementationOnce(() => { | ||
| throw new Error('Checked for executable'); |
Member
There was a problem hiding this comment.
Would have been better to make the executable path configurable and make sure we actually throw on a missing file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The goal of the
symbol-collectortarget is to continue automating the release ofsentry-java(Android and Java). #258 automated publishing to Maven Central, and this PR automates uploading symbols to the Android Bucket.