-
Notifications
You must be signed in to change notification settings - Fork 737
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
add auth_type param to fix issue 1789 #1790
Conversation
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.
Could you add a changelog?
break; | ||
case "basic": | ||
default: | ||
return CURLAUTH_BASIC; |
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.
Before the default value was CURLAUTH_ANY
. I wonder if this change could break some setups?
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 changed the code and now the _getAuthType function by default returns CURLAUTH_ANY.
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 added changelogs
a765f10
to
1188010
Compare
Overall the changes LGTM but it seems like some of the tests need to be updated as CI is failing: https://travis-ci.org/github/ruflin/Elastica/jobs/713737332 |
I fixed the test failures but I do not know why still test has errors |
Thanks for updating the tests. The errors you see now are around linting. Can you run |
The error was fixed. |
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.
Thanks for the fix, LGTM. One last minor comment.
CHANGELOG.md
Outdated
@@ -7,12 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
## [Unreleased](https://github.com/ruflin/Elastica/compare/7.0.0...master) | |||
### Backward Compatibility Breaks | |||
### Added | |||
|
|||
Ability to specify the type of authentication manually by the `auth_type` parameter (in the client class config) was added (allowed values are `basic, digest, gssnegotiate, ntlm`) |
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.
Nit: Sorry, didn't spot this before, but could you prefix it by *
to make it a list? Same for line 16.
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.
Oh you are right
I changed it.
@daalvand Thank you very much for the contribution and going through the iterations! |
I added auth_type option to resolve the problem of duplicate requests in 1789 issue