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

Allow more escalation step wait times over public api #5201

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

mderynck
Copy link
Contributor

@mderynck mderynck commented Oct 23, 2024

What this PR does

Remove restriction on wait times when changing escalation step wait times. 60-86400 seconds values are accepted.

grafana/terraform-provider-grafana#1855 removes the restriction in the terraform provider.

Which issue(s) this PR closes

Related to: https://github.com/grafana/support-escalations/issues/13065

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
    show up in the autogenerated release notes.

@mderynck mderynck added pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes labels Oct 23, 2024
@mderynck mderynck removed the pr:no public docs Added to a PR that does not require public documentation updates label Oct 23, 2024
@mderynck mderynck marked this pull request as ready for review October 23, 2024 13:27
@mderynck mderynck requested a review from a team as a code owner October 23, 2024 13:27
@mderynck mderynck requested review from a team October 23, 2024 13:27
| `position` | Optional | Escalation policies execute one after another starting from `position=0`. `Position=-1` will put the escalation policy to the end of the list. A new escalation policy created with a position of an existing escalation policy will move the old one (and all following) down in the list. |
| `type` | Yes | One of: `wait`, `notify_persons`, `notify_person_next_each_time`, `notify_on_call_from_schedule`, `notify_user_group`, `trigger_webhook`, `resolve`, `notify_whole_channel`, `notify_if_time_from_to`, `declare_incident`. |
| `important` | Optional | Default is `false`. Will assign "important" to personal notification rules if `true`. This can be used to distinguish alerts on which you want to be notified immediately by phone. Applicable for types `notify_persons`, `notify_team_members`, `notify_on_call_from_schedule`, and `notify_user_group`. |
| `duration` | If type = `wait` | The duration, in seconds, when type `wait` is chosen. Valid values are: `60-86400`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe is not that clear enough that any value in the range is valid?

@@ -143,18 +151,10 @@ def to_representation(self, instance):
result = super().to_representation(instance)
result = self._get_field_to_represent(step, result)
if "duration" in result and result["duration"] is not None:
result["duration"] = result["duration"].seconds
result["duration"] = int(float(result["duration"]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this conversion needed? (from what I see in the field definition, the to_representation should return the right repr?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this the DurationSecondsField returns a float as a string:

{
      "position": 0,
      "id": "E6J46FYHBXGUW",
      "escalation_chain_id": "FDWP4BEVEZD9C",
      "type": "wait",
      "duration": "111.0"
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably don't really need to use that DurationSecondsField but I used it to match the other internal API serializer.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm.. right. Sounds ok then 👍

@@ -143,18 +151,10 @@ def to_representation(self, instance):
result = super().to_representation(instance)
result = self._get_field_to_represent(step, result)
if "duration" in result and result["duration"] is not None:
result["duration"] = result["duration"].seconds
result["duration"] = int(float(result["duration"]))
Copy link
Contributor

Choose a reason for hiding this comment

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

hm.. right. Sounds ok then 👍

@mderynck mderynck added this pull request to the merge queue Oct 23, 2024
Merged via the queue into dev with commit 8d9014e Oct 23, 2024
24 of 25 checks passed
@mderynck mderynck deleted the mderynck/allow-variable-wait-times-public-api branch October 23, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:patch PR will be added to "Other Changes" section of release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants