Skip to content
This repository was archived by the owner on Nov 23, 2024. It is now read-only.

Conversation

@adammw
Copy link

@adammw adammw commented Apr 4, 2019

Clone of hashicorp#200 upstream.

/cc @zendesk/compute @zendesk/config

Copy link

@grosser grosser left a comment

Choose a reason for hiding this comment

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

keyword args :D

@adammw
Copy link
Author

adammw commented Apr 4, 2019

I mean, the library isn't using keyword args anywhere else...

@grosser
Copy link

grosser commented Apr 4, 2019

does not hurt to start ... at least don't put optional arguments after options hash

@adammw adammw requested a review from grosser April 4, 2019 23:35
@grosser
Copy link

grosser commented Apr 4, 2019

tests would be good ... but idk what the testing infrastructure looks like here 🤷‍♂️

@adammw
Copy link
Author

adammw commented Apr 5, 2019

so ldap and tls options are completely untested in the integration tests, but added tests for the other ones.

@grosser
Copy link

grosser commented Apr 5, 2019

they all look very similar, can they get moved to a single generic auth method ?

@adammw
Copy link
Author

adammw commented Apr 5, 2019

:shurg:

don't want to deal too much more with this gem right now. this does what i need.

@adammw adammw merged commit cacb44d into master Apr 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants