Repository navigation
BUG: Ignore mirror caching when no internet - #8163
Conversation
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi there @pllim 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
Codecov Report
@@ Coverage Diff @@
## master #8163 +/- ##
==========================================
+ Coverage 86.92% 86.92% +<.01%
==========================================
Files 383 383
Lines 57860 57863 +3
Branches 1056 1056
==========================================
+ Hits 50295 50299 +4
+ Misses 6951 6950 -1
Partials 614 614
Continue to review full report at Codecov.
|
|
Is there any way to write a unittest for this? It looks like there's a |
@drdavella , is there some kind of magical thingies 🦄 I can use from |
|
FWIW, I did test this patch on my laptop with internet turned off. 😅 |
|
|
|
This looks backportable, so I've milestoned as 2.0.10 and will try to remember to double check the behaviour (but having a test doing that for me would be even more awesome) |
Fix relevant test. Add chanhe log.
9c8d5cb to
26223ac
Compare
|
Incidentally, the test I refactored back in #7575 was not quite right (it wasn't exactly testing the thing I designed for it to test). So I fixed it here and also incidentally, that test also somehow tests this issue, because p.s. Given how careless I was at #7575, extra pairs of eyes to check are appreciated! |
|
p.s. @bsipocz , I have a feeling that this might need a manual backport. Just let me know if that is indeed the case, I'll be happy to open another PR straight to 2.x. Thanks! |
|
Yeah, the diff got much larger. I'll definitely ping you if we get there and if that's a bridge we need to cross. |
|
@eteq - any chance for a review here, so this can land in RC2? |
| with urllib.request.urlopen(conf.dataurl, timeout=timeout) as remote: | ||
| _dataurls_to_alias[conf.dataurl] = [conf.dataurl, remote.geturl()] | ||
| except urllib.error.URLError: # Host unreachable | ||
| _dataurls_to_alias[conf.dataurl] = [conf.dataurl] |
There was a problem hiding this comment.
| _dataurls_to_alias[conf.dataurl] = [conf.dataurl] | |
| msg = ('url "{}" was requested in download_file, but was not found.' | |
| ' Will try cached file if it exists.') | |
| warn(AstropyWarning(msg.format(conf.dataurl))) | |
| _dataurls_to_alias[conf.dataurl] = [conf.dataurl] |
There was a problem hiding this comment.
@eteq , personally, I would prefer not emitting a warning. This is because this line was only to cache an alias of the original URL. Hence, user will not find the warning make any sense unless user knows the internal mechanics of this function. If this warning turns out to be necessary, we can always follow up in a future PR. Thank you for your understanding!
|
Thanks @pllim! |
BUG: Ignore mirror caching when no internet
|
f2f decision was to not backport this to LTS. |
5 participants
Address a bug reported by @parejkoj in #7653 where obtaining cached download fails when there is no internet. This does not address the original feature request in #7653 (i.e., providing a user-defined
sites.json).@eteq , can you please review, given you also reviewed #6987 that introduced this bug?
@bsipocz , not sure if this can be cleanly backported to 2.x though #6987 was put in 2.0.4. What should I milestone this to? Do you need a separate PR straight to 2.x branch? Please advise.
Thanks!