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

Refactor time_restriction and stabilise plans containing notification_rule.steps, notification_policy.filter.conditions and time_restrictions #404

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

Baarsgaard
Copy link
Contributor

@Baarsgaard Baarsgaard commented Oct 21, 2023

Commits:

  1. Closes Docs for "opsgenie_notification_policy" contain invalid syntax #246

  2. Closes opsgenie_notification_rule.steps is order sensitive #253 And opsgenie_notification_policy conditions order is not stable causing "in-place" updates though nothing changes #409
    And adds some much needed schema validation for Terraform-ls to use.

  3. Closes Fix for "time restriction" is not stable #341:
    Unifies the Schema, Input-Validation, and expanding/flattening for time_restriction blocks.
    This implementation should fix unstable plans containing time_restriction for all resources.
    Schedule rotation "time restriction" is not stable #225 only addressed a single resource.

  4. Closes opsgenie_notification_policy not validating that all the expected fields are defined #355
    Way stricter input validation of actions before terraform apply

This is a pretty big PR solely because of the time_restriction refactor, I am willing to split it, but I do still think reviewing this PR one commit at a time is feasible.

Restrict Filters and timerestriction blocks on notification policies
Ensure stable plans when using notification rule steps or notification policy conditions
@Baarsgaard
Copy link
Contributor Author

Baarsgaard commented Oct 25, 2023

@koushik-swaminathan Sorry to ping you, but I would love to have this reviewed to continue using the provider :)

@koushik-swaminathan
Copy link
Contributor

@Baarsgaard sure, I'll take a look. Have you tested these changes?

@Baarsgaard
Copy link
Contributor Author

Baarsgaard commented Oct 26, 2023

Yes, all of it is tested.
Luckily very little of the actual business logic has changed.
The only new code is the resourceOpsGenieNotificationPolicyMultiValueValidation() function in the 3rd commit, the rest is refactoring and Schema validation updates.

@koushik-swaminathan
Copy link
Contributor

LGTM, tested it as well. But it looks like it doesn't really fix the conditions in-place issues, but time-restriction seems to be stable now.

@Baarsgaard
Copy link
Contributor Author

Baarsgaard commented Nov 2, 2023

Could you share what specifically does not work?
I've been testing with a fair amount of notification policies and I'm unable to get the perpetual state drift to show up:

Terminal cast

You can see me changing the order of every single condition, both using message and tags
This causes perpetual drift on the current release.

asciicast

@koushik-swaminathan
Copy link
Contributor

koushik-swaminathan commented Nov 3, 2023

Hi @Baarsgaard, the in-place issue was persisting in my local but it seems to work for you. LGTM, approving the PR.

Copy link
Contributor

@koushik-swaminathan koushik-swaminathan left a comment

Choose a reason for hiding this comment

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

Could you edit the date and version in the changelog? I'll merge the PR after that. Thank you!

@Baarsgaard
Copy link
Contributor Author

I'll set it for Monday next week 2023-11-06

@Baarsgaard
Copy link
Contributor Author

@koushik-swaminathan Should be ready for a merge now! :D

@Baarsgaard
Copy link
Contributor Author

LGTM, tested it as well. But it looks like it doesn't really fix the conditions in-place issues, but time-restriction seems to be stable now.

I was thinking about what could be the cause of this.
The difference is likely that the policies I created were saved as a set, but your test was made on policies stored as the old list.

If you run terraform apply -refresh-only followed by a normal terraform apply is the plan still unstable?

Copy link
Contributor

@koushik-swaminathan koushik-swaminathan left a comment

Choose a reason for hiding this comment

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

LGTM!

@koushik-swaminathan koushik-swaminathan merged commit d9700b2 into opsgenie:master Nov 6, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants