Fix monitor delete method#31
Fix monitor delete method#31nickhammond wants to merge 2 commits intocronitorio:masterfrom nickhammond:delete-fix
Conversation
|
|
||
| monitors = opts[:monitors] || [opts] | ||
| url = "https://cronitor.io/api/monitors" | ||
| url = Cronitor::Monitor::API_URL |
There was a problem hiding this comment.
It works in this class method because the string is hardcoded in here.
| attr_reader :key, :api_key, :api_version, :env | ||
|
|
||
| PING_RETRY_THRESHOLD = 3 | ||
| API_URL = 'https://cronitor.io/api/monitors'.freeze |
There was a problem hiding this comment.
Is there a need to make this configurable? Something for internal testing/usage for example.
| "https://cronitor.io/p/#{api_key}/#{key}" | ||
| end | ||
|
|
||
| def monitor_api_url |
There was a problem hiding this comment.
No longer needed since instance and class methods now just utilize the constant.
|
Anything else needed here? |
|
Hi @nickhammond thanks so much for bringing this up, and my apologies for the slow reply, I had accidentally disabled notifications for this repo. I just released version 5.2.2 which addresses this. However, the intention originally was that .delete should be an instance method not a class method (this aligns with the pattern that the rest of our SDKs follow), so the real bug was that it had been created as a class method. I've fixed that, and will work as documented now. I'm going to close this out, but thank you again for bringing this to my attention! |
|
@aflanagan No worries, thanks for releasing the new version with this fix! |
2 participants
.deleteis a class method andmonitor_api_urlisn't defined on the class or the config, just on an instance of a monitor. I moved the API URL up to a constant and removed the instance method so that it can be utilized from a class or instance method.You can see the failing call to
deletewhen you run specs locally with an API key, you'll get: