-
Notifications
You must be signed in to change notification settings - Fork 31
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 multiple endpoints support #87
Conversation
Looks good! Let's make the tests pass once the new dogapi gem is released :) I've found a couple of other places that'll need an update too:
|
Also, let's not forget to update the version constraint on |
b2922d9
to
a910c47
Compare
end | ||
|
||
# If not enough app_keys compared to api_keys | ||
if app_keys.nil? || app_keys.length != api_keys.length |
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.
Do you need to re-test if app_keys.nil?
? It would have failed above
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.
You're right, nice catch.
Looks good to me, and well tested 💚 . |
a910c47
to
3193f13
Compare
# Validate endpoints config (urls, api_keys and application keys) | ||
# fails if incorrect and nothing can be done | ||
# Doesn't fail if at least one endpoint can be used | ||
def validate_extra_endpoints(urls, api_keys, app_keys) |
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.
naming nitpick: I'd prefer validate_endpoints
since it doesn't apply only to the extra endpoints
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.
😮 , leftover from the previous version
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.
@degemer Looks like you forgot to rename this method
Let's merge this once the tests pass! 🎆 |
@degemer Could you also rebase the PR please? All the other code changes that'll ship with the next minor release have been merged to master. |
It uses the new dogapi feature to do so. Tests are failing because the new dogapi gem with multiple endpoints support is not yet released.
9b4497b
to
8d79aea
Compare
Yay, rebased, fixed the nits, finally ready to 🚀 ! Last review @olivielpeau ? |
@degemer Let's rename |
8d79aea
to
3808f85
Compare
Humpf, I know why I forgot it, but this is vexing. 🔥 |
3808f85
to
872916d
Compare
dogs | ||
end | ||
|
||
def convert_to_array(value_or_array) |
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.
this is not used anymore, let's remove it :)
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.
🔥
Had 2 more comments for your consideration, apart from that I think we're getting there :) |
The handler handles the multiple endpoints issues, since it has all the information. Require dogapi >= 1.23.0 for the endpoint configuration.
872916d
to
ee6832e
Compare
Thanks for the review, updated the PR! |
LGTM! Let's merge once DataDog/chef-datadog#317 is ready too |
It uses the new dogapi feature to do so.
Tests are failing because the new dogapi gem with multiple endpoints
support is not yet released.