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

enforce that composite monitors use links #197

Merged
merged 1 commit into from
Sep 29, 2022
Merged

Conversation

grosser
Copy link
Owner

@grosser grosser commented Sep 27, 2022

No description provided.

@zdrve
Copy link
Collaborator

zdrve commented Sep 27, 2022

Observations (not necessarily problems):

  1. It's very much a non-backwards-compatible change (mainly of interest to anyone using Kennel who isn't Zendesk)
  2. If, as someone using kennel outside of Zendesk, you wanted to turn off this behaviour that this PR adds – i.e. you're fine with link-by-datadog-id, without the need to opt out each time – how would you do that?
  3. The only way to opt out is to opt out of all validations for the monitor.
  4. How is the mutable overridable array meant to be used? Can you sketch what that would look like for Zendesk's case?

@grosser
Copy link
Owner Author

grosser commented Sep 27, 2022

1: it requires some code changes, but not too bad I'd hope
2: override the method in the monitor class to be noop
3: it's a setting so link validation can be turned off on it's own
4: ALLOWED_UNLINKED = YAML.load_file('our_allow_list.yam') or hard-coded list in the code

@@ -238,6 +239,8 @@ def validate_json(data)
validate_message_variables(data)
end

validate_using_links(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer a boolean setting:

Suggested change
validate_using_links(data)
validate_using_links(data) if require_using_links

Having a "setting" where the two values are "method with logic" or "override with a no-op block" is ... a weird way of programming.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Particularly when the method/block in question effectively returns void

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah it's not very smooth ... some more general skip_validations: [:foo] might be nicer ... but that's more refactoring and can be it's own PR

Copy link
Owner Author

Choose a reason for hiding this comment

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

another option would be

eval "->(*) { v }", nil, file, line.to_i

to use validate_using_links: false
but that ends up making any setting callable which could end up ugly too

@@ -27,11 +27,12 @@ class Monitor < Record
}.freeze
DEFAULT_ESCALATION_MESSAGE = ["", nil].freeze
ALLOWED_PRIORITY_CLASSES = [NilClass, Integer].freeze
ALLOWED_UNLINKED = [] # rubocop:disable Style/MutableConstant placeholder for custom overrides
Copy link
Collaborator

Choose a reason for hiding this comment

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

If (when) we extend this validation to other linking types - e.g. monitor widgets in dashboards – what will ALLOWED_UNLINKED look like then?

Copy link
Owner Author

Choose a reason for hiding this comment

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

will need to move up into Kennel::ALLOWED_UNLINKED but the structure can stay the same

@grosser grosser merged commit 052f764 into master Sep 29, 2022
@grosser grosser deleted the grosser/composite branch September 29, 2022 21:52
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.

2 participants