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

schedule: Validate daily/weekly restrictions #439

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zecke
Copy link
Contributor

@zecke zecke commented Dec 24, 2021

Do not allow a daily restriction that spans a full 24time.Hour
and similar for weekly restrictions. The PD API rejects 24
time.Hour
and 247time.Hour for the API.

zecke and others added 2 commits December 24, 2021 19:00
Do not allow a daily restriction that spans a full 24*time.Hour
and similar for weekly restrictions. The PD API rejects 24*time.Hour
and 24*7*time.Hour for the API.
Copy link
Contributor

@stmcallister stmcallister left a comment

Choose a reason for hiding this comment

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

Oooo! I like this solution! It's really readable AND you have tests!!! 😍 I just merged #433 which accomplishes the same thing, but I would also like this one because of readability and the included tests. However, when I run the tests with the newly merged code the test fails with the following message. If you could fix this I would love to merge it in to the project.

=== RUN   TestAccPagerDutySchedule_BasicWeek
    resource_pagerduty_schedule_test.go:216: Step 3/3, expected an error with pattern, no match on: Error running pre-apply refresh: exit status 1
        
        Error: expected layer.0.restriction.0.duration_seconds to be in the range (1 - 604799), got 604800
        
          with pagerduty_schedule.foo,
          on terraform_plugin_test.tf line 23, in resource "pagerduty_schedule" "foo":
          23:       duration_seconds  = 604800
        
--- FAIL: TestAccPagerDutySchedule_BasicWeek (12.00s)

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