Skip to content
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 options for force delete monitors #213

Merged
merged 3 commits into from
Jan 14, 2020

Conversation

iminoso
Copy link
Contributor

@iminoso iminoso commented Jan 13, 2020

https://datadoghq.atlassian.net/browse/MA-791

What does this PR do?

Allow monitors to be force deleted even if part of composite or SLO via options

Description of the Change

Enabled options to be passed in the monitor_delete function in the client, to allow for force deletion.

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@iminoso iminoso marked this pull request as ready for review January 13, 2020 20:48
@iminoso iminoso requested a review from a team as a code owner January 13, 2020 20:48
# Cop supports --auto-correct.
Layout/EmptyLineAfterMagicComment:
Exclude:
- 'dogapi.gemspec'
- 'spec/integration/comment_spec.rb'
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran the rubocop autogenerate command rubocop --auto-gen-config with the ruby version that's used for linting so I think this should be good

@@ -549,8 +549,8 @@ def can_delete_monitors(monitor_ids)
@monitor_svc.can_delete_monitors(monitor_ids)
end

def delete_monitor(monitor_id)
@monitor_svc.delete_monitor(monitor_id)
def delete_monitor(monitor_id, options = {})
Copy link
Contributor

@nmuesch nmuesch Jan 13, 2020

Choose a reason for hiding this comment

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

This means existing users that aren't passing in options will still be able to omit it, right? I.e. we're backwards compatible here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's a default argument so it's backwards compatible!

@@ -51,6 +51,17 @@
:delete, "/monitor/#{MONITOR_ID}"
end

describe '#delete_monitor with options' do
Copy link
Contributor

Choose a reason for hiding this comment

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

💜 Thanks

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@nmuesch nmuesch merged commit 754758b into master Jan 14, 2020
@nmuesch nmuesch deleted the ian.minoso/add-options-delete-monitor-ruby branch January 14, 2020 04:57
@zippolyte zippolyte changed the title [monitors][api] MA-791 Add options to ruby client for force delete monitors Add options for force delete monitors Feb 4, 2020
@zippolyte zippolyte added the changelog/Added Added features results into a minor version bump label Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants