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

Remove locked attribute #311

Merged
merged 2 commits into from
Mar 29, 2024
Merged

Conversation

brianburnszd
Copy link
Contributor

@brianburnszd brianburnszd commented Mar 29, 2024

According to Datadog docs, locked attribute in monitors is no longer supported.

This PR:

  1. Stops sending locked as an attribute when creating monitors
  2. No longer diffs locked attribute when comparing monitors against Datadog

Tested out locally and shows the API request to create monitors without locked attr:

{
  "name": "Test Locked attr",
  "tags": ...
  "options": {
    ...
    <no "locked" option>
    }
}

Note that when merged, all existing monitor generated JSON will also loose the locked attribute, but because the diff is a no-op, Datadog will not be updated (what we want).

Checklist

  • Verified against local install of kennel (using path: in Gemfile)
  • Added tests
  • Updated Readme.md (if user facing behavior changed)

this is basically a no-op since the attribute locked is already removed
@brianburnszd
Copy link
Contributor Author

brianburnszd commented Mar 29, 2024

This may be a breaking change to anyone using locked: true in monitor configuration. Not sure how this needs to be communicated or if it needs to be noted in a Changelog? Probably requires a major version update?

@grosser
Copy link
Owner

grosser commented Mar 29, 2024 via email

@grosser
Copy link
Owner

grosser commented Mar 29, 2024

readonly did not work since it's a option and not a top-level attribute

@grosser grosser merged commit 867b386 into grosser:master Mar 29, 2024
4 checks passed
@grosser
Copy link
Owner

grosser commented Mar 29, 2024

I'll add another small fix and then make zendesk PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants