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

60 handle errors from non http url sources #61

Merged
merged 22 commits into from
Nov 6, 2023

Conversation

EddyCMWF
Copy link
Contributor

ftp is not compatible with multiurl, so if ftp we use the wget downloader.
Hopefully the approach is generic to facilitate other downloaders in the future.

@EddyCMWF EddyCMWF requested a review from malmans2 October 27, 2023 14:46
@EddyCMWF EddyCMWF linked an issue Oct 27, 2023 that may be closed by this pull request
@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Nov 1, 2023

Hi Mattia,
I've fixed and released multiurl which addresses the issue upstream of cads-adaptors.
I've also added an ftp download test.
So good for a re-review.
Eddy

@EddyCMWF EddyCMWF linked an issue Nov 1, 2023 that may be closed by this pull request
@malmans2
Copy link
Member

malmans2 commented Nov 1, 2023

I'll take a look tomorrow (bank holiday in Italy today).
I you didn't notice it, the other day I opened a PR to this branch to test both ftp ad http: #65
It also fixes this #61 (comment)

Copy link
Member

@malmans2 malmans2 left a comment

Choose a reason for hiding this comment

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

Hi @EddyCMWF,

I have a few comments. I'm approving so you can merge when you are happy with it.

About the tests: As I mentioned in the comments, I'm a bit worried that the library used in this PR is not stable/reliable.
Also, IMO it's better if we don't rely on external files. That way we don't need internet access to run the tests, we don't download external files every time tests are executed, and we are not affected by the external data (e.g., urls could change, downtimes, ...)
Given that the ftp fixture is just about 10 lines of code, I've aligned another branch to this PR in case you change your mind and you want to use my version of the tests: #66

@EddyCMWF EddyCMWF merged commit 7496113 into main Nov 6, 2023
@EddyCMWF EddyCMWF deleted the 60-handle-errors-from-non-http-url-sources branch November 6, 2023 10:19
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.

handle Errors from non http URL sources Adaptor error on satellite-sea-surface-temperature
2 participants