-
Notifications
You must be signed in to change notification settings - Fork 8
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
http: dies on platform if no credential #733
Comments
@josephjclark Does this still need fixing ?, I am seeing a much better error on Eg: |
@mtuchi was something done in a recent release to address this? Bc in |
From our pairing session with @aleksa-krolls, Tested on v2 a simple job without credentials using http adaptor get('/jokes/random')
get('https://api.openconceptlab.org/mappings/') |
We know it works on v2, the problem is only seen on v1. It doesn't matter that we get a nice error - the problem is it shouldn't error at all. So yes, this still needs fixing unfortunately |
@josephjclark But isn't Regarding v2 i was pairing with @aleksa-krolls yesterday, She mentioned she faced something similar on v2 but we couldn't reproduce it during the call. The only error we were getting is |
Hmm. It's a badly written issue and I only barely remember it. But I think the problem is that
|
Ah I agree with Joe @mtuchi ... didn't think about this use case, but some jobs do not require authentication and no credential... so that should be allowed. |
EOD Update #772 |
Thanks @mtuchi, that's cool, but that PR doesn't fix the actual issue here. You need to change:
To this (or some equivalent:
To understand what's happening, try this test in
|
@josephjclark i have updated the addAuth in |
The http adaptor does this:
(probably other adaptors too)
This blows up on v1 if there's no credential, because v1 passes
null
as the credential. Andnull
is a value, it won't trigger the default.While we're still supporting v1, we need to be careful about defaulting the
configuration
object across our adaptors.we should really fix this in http because
latest
will be forever broken on v1, which isn't so goodThe text was updated successfully, but these errors were encountered: