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

feat: improve PD service time window validation to accept 86400 as a valid value #876

Merged

Conversation

boneff
Copy link
Contributor

@boneff boneff commented May 27, 2024

Aims to fix: #849

@boneff boneff force-pushed the improve-time-window-validation branch 3 times, most recently from 1e0ec8f to 786c48b Compare May 27, 2024 09:36
@boneff boneff marked this pull request as ready for review May 27, 2024 09:40
@boneff boneff changed the title [WIP] feat: improve PD service time window validation to accept 86400 as a valid value feat: improve PD service time window validation to accept 86400 as a valid value May 27, 2024
@imjaroiswebdev
Copy link
Contributor

Hi @boneff thank you for your contribution, is a quite complete PR, which also includes the necessary acceptance tests. We appreciate it. However this input value is not currently supported by pagerduty_service.alert_grouping_parameters.0.config.0.time_window as I stated here.

Therefore, I'm proceeding to close this PR for the moment, if the input range gets updated to support what this PR is aiming to add, then We'll considere it as an update to bring support for the new input range.

@imjaroiswebdev
Copy link
Contributor

Hi @boneff you were right about this update, because when I reviewed this improvement, I checked the valid input range for Intelligent Alert Grouping, which is limited to 3600 max. However, this update targets Content Based Alert Grouping input range, which indeed supports 300 <= time_window <= 3600 or 86400(i.e. 24 hours).

So, I'm reopening and reviewing it again. Sorry for the mixup btw 😓

@imjaroiswebdev imjaroiswebdev force-pushed the improve-time-window-validation branch from 786c48b to 8fb9df8 Compare June 14, 2024 16:57
Copy link
Contributor

@imjaroiswebdev imjaroiswebdev left a comment

Choose a reason for hiding this comment

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

Thank you adding this improvement @boneff. I took the liberty of rebasing and updating the documentation.

@imjaroiswebdev imjaroiswebdev merged commit 990b63d into PagerDuty:master Jun 14, 2024
1 check passed
@boneff
Copy link
Contributor Author

boneff commented Jun 16, 2024

Thank you adding this improvement @boneff. I took the liberty of rebasing and updating the documentation.

Thank you for reconsidering @imjaroiswebdev!
Any idea when that change would be released ?

@imjaroiswebdev
Copy link
Contributor

Hi @boneff is already available in version latest version of PagerDuty TF provider, it was released more precisely in version v3.14.0

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.

Unable to define time_window values > 1h
2 participants