Bcon/make query optional monitor update#192
Conversation
| body = { | ||
| 'query' => query, | ||
| }.merge options | ||
| body = {}.merge options |
There was a problem hiding this comment.
Rather than merging an empty hash, I would write it as :
def update_monitor(monitor_id, query, options)
unless query.nil?
options['query'] = query
warn '[DEPRECATION] query param is no longer required for update'
end
request(Net::HTTP::Put, "/api/#{API_VERSION}/monitor/#{monitor_id}", nil, options, true)
endThere was a problem hiding this comment.
For some reason it's being picky about the order and local tests don't pass with this
¯_(ツ)_/¯
|
One thing I forgot is you'll need to bump the version here. Just bumping it up to |
lib/dogapi/v1/monitor.rb
Outdated
| body = { | ||
| 'query' => query | ||
| }.merge body | ||
| warn '[DEPRECATION] query param is no longer required for update' |
There was a problem hiding this comment.
I would not warn here. If someone does want to actually update the query, they should not see a deprecation message.
There was a problem hiding this comment.
Actually you were right to have this message. I misunderstood how all of it worked.
There was a problem hiding this comment.
Ok just put it back in with the updates!
zippolyte
left a comment
zippolyte
left a comment
There was a problem hiding this comment.
Final 👍, I think we are good to go for real 🙂
|
@zippolyte Great thanks for the feedback! |
3 participants
Similar to DataDog/datadogpy#447
Made query an optional param. Tests pass locally, but happy to add some new testing if appropriate.