Fix repack AAB output path#3892
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3892 +/- ##
==========================================
+ Coverage 59.01% 59.06% +0.05%
==========================================
Files 935 935
Lines 40933 40964 +31
Branches 8624 8629 +5
==========================================
+ Hits 24153 24190 +37
+ Misses 16684 16678 -6
Partials 96 96 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Subscribed to pull request
Generated by CodeMention Warning: The preamble and epilogue options in commentConfiguration are deprecated. Use template instead. |
| expect(repackStep.outputById['output_path'].value).toBe('/path/to/output_app'); | ||
| }); | ||
|
|
||
| it('should rename explicit aab output path to apk', async () => { |
There was a problem hiding this comment.
isn't this weird? like what's the purpose of output_path input if it can be different in the end haha
maybe we can drop the input altogether?
There was a problem hiding this comment.
that's kind of intentional to support aab. repack wouldn't be ideal tool to support production aab build, but it's fine to repack from aab source build.
open to idea if there's a better way to support aab. (or even fine that just not to support aab)
There was a problem hiding this comment.
i'm talking only about output_path being both an input and an output. i'm all for repacking an aab. if i were to pass in some value to output_path as input i would expect the artifact to be where i told it to be
There was a problem hiding this comment.
i got your point! in the latest commit, it preserves the output_path if explicitly specifying an output_path.
sjchmiela
left a comment
sjchmiela
left a comment
There was a problem hiding this comment.
thank you!
we could also add Datadog.log for when we do see output_path being used (and not used), and if it isn't, remove it?
| const outputPath = createOutputPath({ | ||
| requestedOutputPath: inputs.output_path.value as string | undefined, | ||
| sourceAppPath, | ||
| tmpDir, | ||
| }); |
There was a problem hiding this comment.
| const outputPath = createOutputPath({ | |
| requestedOutputPath: inputs.output_path.value as string | undefined, | |
| sourceAppPath, | |
| tmpDir, | |
| }); | |
| const outputPath = inputs.output_path.value as string | undefined || createOutputPath({ | |
| sourceAppPath, | |
| tmpDir, | |
| }); |
it feels clearer to me
i think output_path is not used. want me to remove it in this pr? |
|
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |
now i entirely remove the |
2 participants
Why
Repacked Android builds should produce an APK output path when the source app path ends in
.aab.How
Removed the unused
output_pathinput and generate the repacked output path internally. Generated.aabpaths are rewritten to.apk.Test Plan
yarn run -T oxfmt packages/build-tools/src/steps/functions/repack.ts packages/build-tools/src/steps/functions/__tests__/repack.test.tsyarn --cwd packages/build-tools jest-unit repack.test.ts --watchman=false