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

Handle curl failure #78

Merged
merged 7 commits into from
Sep 11, 2023
Merged

Handle curl failure #78

merged 7 commits into from
Sep 11, 2023

Conversation

andreabedini
Copy link
Member

As we discovered, curl does not fail on http response codes >=400 by default.
This PR:

  • adds --fail to curl's flags
  • uses curl's --write-out to obtain a decent error message
  • improves some error messages.

Sample output:

❯ cabal run -- foliage build
🌿 Foliage
# buildAction
Current time set to 2023-09-05T04:06:10Z. You can set a fixed time using the --current-time option.
# _sources/pkg-a/1.2/meta.toml
# curl (for fetchRemoteAsset https://httpbin.org/status/500)
foliage: Error when running Shake build system:
  at want, called at app/Foliage/CmdBuild.hs:50:7 in main:Foliage.CmdBuild
* Depends on: buildAction
  at apply1, called at app/Foliage/PrepareSource.hs:39:31 in main:Foliage.PrepareSource
* Depends on: prepareSource pkg-a-1.2 PackageVersionSpec {packageVersionTimestamp = Nothing, packageVersionSource = TarballSource {tarballSourceURI = https://httpbin.org/status/500, subdir = Nothing}, packageVersionRevisions = [], packageVersionDeprecations = [], packageVersionForce = False}
  at apply1, called at app/Foliage/RemoteAsset.hs:37:20 in main:Foliage.RemoteAsset
* Depends on: fetchRemoteAsset https://httpbin.org/status/500
  at error, called at app/Foliage/RemoteAsset.hs:108:19 in main:Foliage.RemoteAsset
* Raised the exception:
The requested URL returned error: 500

Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@angerman angerman left a comment

Choose a reason for hiding this comment

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

Looks sensible.

app/Foliage/RemoteAsset.hs Show resolved Hide resolved
@michaelpj
Copy link
Collaborator

What are we doing about the CI here? Re-evaluating doesn't seem to be having any effect :think

@angerman
Copy link

angerman commented Sep 6, 2023

What are we doing about the CI here? Re-evaluating doesn't seem to be having any effect :think

I've asked @cleverca22 for some help. I need to get some windows idiocity done before I can focus on something else :-(

@andreabedini andreabedini merged commit ea68345 into main Sep 11, 2023
2 checks passed
@andreabedini andreabedini deleted the andrea/handle-curl-failure branch September 11, 2023 05:18
@andreabedini andreabedini self-assigned this Sep 11, 2023
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.

4 participants