search

opsgenie / terraform-provider-opsgenie

Terraform OpsGenie provider
https://registry.terraform.io/providers/opsgenie/opsgenie/latest/docs
Mozilla Public License 2.0
100 stars 135 forks source link

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

Closed Baarsgaard closed 10 months ago

Baarsgaard commented 10 months ago

Commits:

  1. Closes #246

  2. Closes #253 And #409 And adds some much needed schema validation for Terraform-ls to use.

  3. Closes #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.

    225 only addressed a single resource.

  4. Closes #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.

Baarsgaard commented 10 months ago

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

koushik-swaminathan commented 10 months ago

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

Baarsgaard commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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](https://asciinema.org/a/trZYQYc4jusQN7limWlXVozV1.svg)](https://asciinema.org/a/trZYQYc4jusQN7limWlXVozV1)
koushik-swaminathan commented 10 months ago

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

Baarsgaard commented 10 months ago

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

Baarsgaard commented 10 months ago

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

Baarsgaard commented 10 months ago

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?