-
Notifications
You must be signed in to change notification settings - Fork 151
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
i.sentinel.download: use EODAG as a unified API for downloading sentinel products #1150
Conversation
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
It seems a big simplification, as soon it will be ready to test let us know. |
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
@neteler @ninsbl |
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
6628896
to
d32374b
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.
I left a couple of comments. Would you mind updating the manual page too, so it's easier to test?
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
Thanks @veroandreo, I will update the manual ASAP. |
S3 OL 1 RAC is an internal product. Perfectly fine to drop it... Not sure what the background of the SAC product has been. But I did not find documentation either. So fine to drop as well... |
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.
A general question is how backwards-compatibility is handled or if we should put a lot of effort into it?
Some features will disappear in this PR (e.g. possibility to download from DHUS portals = sentinelsat).
Personally, I would give priority to basic tests in i.eodag...
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.py
Outdated
Show resolved
Hide resolved
Thanks for the review, @ninsbl.
I will be working on that in parallel with this PR now.
Is there any points that I might have not recognized? |
For me it is no problem if the format of printed info changes a bit after switching to eodag... Not an issue I would invest a lot of time in. As mentioned, I think there will be breaking changes anyway... |
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.html
Outdated
Show resolved
Hide resolved
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.html
Outdated
Show resolved
Hide resolved
src/imagery/i.sentinel/i.sentinel.download/i.sentinel.download.html
Outdated
Show resolved
Hide resolved
30039b6
to
45142a0
Compare
There are unresolved conversations here, a merging conflict and then we need an approval to merge. Can you please check the unresolved conversations and merge conflict @HamedElgizery ? After that I'd merge and do a second round of review once all is in |
As @veroandreo mentioned, backwards compatibility is not that a hard requirement for addons, But I think users should get a heads-up (GRASS-dev / GRASS-users ML) before breaking changes are submittet. |
I agree we can do a courtesy heads up, but that's not common practice for addons really. COAH and sentinelsat stopped working almost one year ago, so users of i.sentinel should already know, I suppose. I am moreover for merging this, update the manual with a notice about COAH and sentinelsat not working anymore, and sending an email afterwards. |
No objections from my side... |
Please wait for OSGeo/grass#4205 to be merged, then restart the failing job. |
This PR will reimplement i.sentinel.download making use of the i.eodag module to use the EODAG API as the main source of Sentinel products.