-
Notifications
You must be signed in to change notification settings - Fork 6
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
Im 542 get s3 region by analyzing the asset href #224
Im 542 get s3 region by analyzing the asset href #224
Conversation
As we need to check the protocol for href, we cannot rely on this extension anymore.
} catch (Exception e) { | ||
System.err.println("Exception testing region " + region + ": " + e.getMessage()); | ||
// Continue testing other regions if the bucket is not found | ||
continue; |
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.
This catch is very general, could it be interesting to write a warning to the log file?
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.
I am not sure about adding the logging class here, as the loop will fail most times. I could manage the exceptions in a more specialized way.
return resolvedRegion; | ||
|
||
} catch (Exception e) { | ||
System.err.println("Failed to resolve region dynamically for bucket: " + bucketName + ". Error: " + e.getMessage()); | ||
// try another one |
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.
This catch is very general, could it be interesting to write a warning to the log file?
Added mock for Monitor
Changelog:
item_assets
compatibility, as it lacked thehref
field needed for getting the bucket.s3BucketRegion
toawsRegion
and removed it as a parameter at the importer.