Skip to content
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

task/runner: Handle extra import request errs when humanizing #104

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

yondonfu
Copy link
Member

Fixes #103

@yondonfu yondonfu requested a review from a team as a code owner December 14, 2022 17:45
@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #104 (7ae097f) into main (66ef87d) will decrease coverage by 0.06841%.
The diff coverage is 0.00000%.

Impacted file tree graph

@@                Coverage Diff                @@
##               main       #104         +/-   ##
=================================================
- Coverage   5.45262%   5.38421%   -0.06841%     
=================================================
  Files            14         14                 
  Lines          1889       1913         +24     
=================================================
  Hits            103        103                 
- Misses         1777       1801         +24     
  Partials          9          9                 
Impacted Files Coverage Δ
task/runner.go 5.02513% <0.00000%> (-0.32246%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 044f276...7ae097f. Read the comment docs.

Impacted Files Coverage Δ
task/runner.go 5.02513% <0.00000%> (-0.32246%) ⬇️

@@ -475,9 +475,19 @@ func errorInfo(err error) *data.ErrorInfo {
func humanizeCatalystError(err error) error {
errMsg := strings.ToLower(err.Error())

fileNotAccessibleErrs := []string{
"504 Gateway Timeout",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be lower case as well

// General errors
if strings.Contains(errMsg, "import request") && strings.Contains(errMsg, "504 Gateway Timeout") {
return errors.New("file could not be imported from URL because it was not accessible")
// TODO(yondonfu): This string matching is ugly and we should come up with a better less error-prone way to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We should probably work with error codes like mediaconvert does (but may always have some string parsing fallback anyway, just hopefully won't be the main logic)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve handling of Catalyst error messages for inaccessible file when humanizing
3 participants