Skip to content

Conversation

@ghyatzo
Copy link
Contributor

@ghyatzo ghyatzo commented Jan 15, 2025

This is rather minor and left for last.

It simply adds a check to grab the error in case of a badly formatted request that cuts down on the spam that HTTP.jl does, and provides a more descriptive error other than "422: unprocessable event"

Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Needs tests.

fix: added tests
ghyatzo and others added 2 commits January 15, 2025 18:11
@juliohm
Copy link
Member

juliohm commented Jan 15, 2025

Tests are failing on GitHub Actions for some reason. Appreciate if you could investigate further.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 16, 2025

There seems to be something wrong with the setup of the api keys.

The error is about the request url not being a valid uri. It seems creds["url"]is empty.

@juliohm
Copy link
Member

juliohm commented Jan 16, 2025 via email

@juliohm
Copy link
Member

juliohm commented Jan 16, 2025

I've edited the README in the master branch, and CI passed successfully. Could this be caused by the fact that the PR is from a fork?

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 16, 2025

Could this be relevant: https://stackoverflow.com/questions/73866908/github-actions-main-repository-secret-not-picked-up-from-pull-request-build ?

from the logs it really seems like ${{ secrets.CDSAPI_URL }} returns an empty string, so maybe Pull requests are not set up to see the secrets?

@juliohm
Copy link
Member

juliohm commented Jan 16, 2025

That is interesting. I will test locally and merge if tests are ok 👍🏽

@juliohm juliohm merged commit 5b5b932 into JuliaClimate:master Jan 16, 2025
0 of 6 checks passed
@ghyatzo ghyatzo deleted the check-bad-request branch January 16, 2025 12:11
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.

2 participants