-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Added build summary with list of packages that were pushed #253
Conversation
hnrkndrssn
commented
Jul 14, 2022
•
edited
Loading
edited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So beautiful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
async pushPackage(): Promise<void> { | ||
info('🔣 Parsing inputs...') | ||
const cliLaunchConfiguration = this.generateLaunchConfig() | ||
|
||
const options: ExecOptions = { | ||
listeners: { | ||
stdline: input => this.stdline(input) | ||
stdline: async input => await this.stdline(input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExecOptions.stdline expects a void function, so it will ignore the Promise here and won't await it.
In practice it's probably OK, but worst-case scenario is that perhaps lines could get written out of order because it will call stdline in quick succession while previous async tasks are floating in the background?
The async part seems to be because you used the GitHub actions markdown summary builder thing.
Suggest keeping stdline as a sync function, and just collect all the stuff into an array, and call createBuildSummary later on after the process has completed.