-
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: when mediaconvert job fails log message #78
base: main
Are you sure you want to change the base?
Conversation
e5c0fc5
to
fd49bae
Compare
fd49bae
to
883f2ba
Compare
Should rethink when to check for job error. Current suggestion only considers during job creation. |
7289763
to
db0b55f
Compare
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.
Looks good, added some comments about cosmetic issues.
src/pipelines/aws/aws-pipeline.ts
Outdated
logger.error(`No Job from create response ${inputFilename}`); | ||
return ''; | ||
} | ||
const s3Status = await this.loopGetJobStatusUntilFinished( |
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.
Name of variable could be better, ie mediaConvertJobStatus
or so.
src/pipelines/aws/aws-pipeline.ts
Outdated
} | ||
} | ||
|
||
async delay_ms(ms: number): Promise<void> { |
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.
Since this function is pretty generic and not really tied to the aws-pipeline, it could make sense to move it somewhere else, utils.ts
or something. Also for consistency the name should be delayMs
or just delay
.
db0b55f
to
ce651aa
Compare
It would be good to have mediaconvert error messages be visible in the console.