-
Notifications
You must be signed in to change notification settings - Fork 134
Catch exceptions whilst trying to fetch metadata #325
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
Conversation
07458d7 to
87ef717
Compare
f-f
left a comment
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.
Hi @carlosdagos, thanks for this! Looks good already, I just left a comment in case you'd wish to use the metadata too 🙂
| pure meta | ||
| pure mempty) | ||
| (do | ||
| metaBS <- Http.getResponseBody `fmap` Http.httpBS metaURL |
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.
If you'd like to be able to customize the location of the metadata (because e.g. you have a proxy to GitHub in your network, or you could download it locally, etc) we could try to read it from environment, e.g. something like:
| metaBS <- Http.getResponseBody `fmap` Http.httpBS metaURL | |
| metaBS <- Dhall.input Dhall.lazyText ("env:SPAGO_METADATA_JSON ? " <> metaURL <> " as Text") |
This won't compile because types mismatch but just to give you an idea. This Dhall string:
- tries to read the
SPAGO_METADATA_JSONvariable from environment - if that's missing, it tries to import the default URL
as Text - imports the result as a lazy
Text, so then we can parse it
When setting the SPAGO_METADATA_JSON variable, you could put there something like https://some.proxy/metadata.json as Text or ./local/metadata.json as Text or even "{ \"some\": \"escaped json\" }"
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 really like this idea. Will experiment with it a bit 👍
The previous way was only handling the case where the metadata fetched could not be correctly parsed. Now it will also disable the global cache if the HTTP request fails for any reason.
4fb9a19 to
cc04d53
Compare
|
@carlosdagos I'll merge this before it gets too out of sync with Thank you! |
Description of the change
The previous way was only handling the case where the metadata fetched
could not be correctly parsed. Now it will also disable the global
cache if the HTTP request fails for any reason.
NOTE
I don't know if I'm on to the right thing here. In my build environment
the metadata URL isn't reachable due to firewalls (and I can't change that
in the short term, unfortunately). Maybe this isn't the right change and
I should open an issue to track all issues related to my use case 😄
Thanks for
spago! I'm really enjoying using it 👍Checklist:
READMEP.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊