1.10.1 should be 2.0.0 - Githubissues

ghost commented 3 years ago

Hi there,

Thank you for opening an issue. Please note that we try to keep the Terraform issue tracker reserved for bug reports and feature requests. For general usage questions, please see: https://www.terraform.io/community.html.

Terraform Version

Terraform v1.0.4
on darwin_amd64
+ provider registry.terraform.io/pagerduty/pagerduty v1.10.1

Affected Resource(s)

pagerduty_schedule#teams

Terraform Configuration Files

not needed.

Debug Output

  # pagerduty_schedule.tridem_pl will be updated in-place
  ~ resource "pagerduty_schedule" "tridem_pl" {
        id          = "PI6APPU"
        name        = "Tridem_Schedule_PL"
      ~ teams       = [
            "P1V3PP8",
          - "PZPZPPD",
        ]
        # (2 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Panic Output

none.

Expected Behavior

Release 1.10.1 should be 2.0.0. I believe that backward compatibility is broken because if I run the same code with 1.10.1 as I did with 1.10.0 I think something breaks or its not clearly documented. I have to adapt code now and that normally is not the case for patches.

Actual Behavior

One of two things happen:

A I apply exactly the same state that I have now, effectively team associations. The result I do not know (maybe PD backend immediately recreates them) or something gets broken. What happens when I remove this team association? It is not described. Also I feel like this internal data legacy should not be reflected in the resource files, it should be abstracted with some ignore if it must be there.

B I change all our schedules.

Steps to Reproduce

Have a schedule without teams, upgrade the provider, run plan and see that teams must be added to recreate original state.

Important Factoids

I am not sure if the PD API also changed as I observe the same behaviour with 1.10.0 even since 1.10.1 came out.

References

I talked to you in SAP-PD Slack about that.

ghost commented 3 years ago

I now ran

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # pagerduty_schedule.fallback will be updated in-place
  ~ resource "pagerduty_schedule" "fallback" {
        id          = "PCTVEUP"
        name        = "S4-SAM-FSM-Fallback_Schedule"
      ~ teams       = [
          - "P3Y7DTM",
          - "P1V3E98",
        ]
        # (2 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

  # pagerduty_schedule.tridem_eng1 will be updated in-place
  ~ resource "pagerduty_schedule" "tridem_eng1" {
        id          = "P2JCXQA"
        name        = "S4-SAM-FSM-Tridem_Schedule_Eng1"
      ~ teams       = [
            "P1V3E98",
          - "PZPZUBD",
        ]
        # (2 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

  # pagerduty_schedule.tridem_eng2 will be updated in-place
  ~ resource "pagerduty_schedule" "tridem_eng2" {
        id          = "PLFLXAH"
        name        = "S4-SAM-FSM-Tridem_Schedule_Eng2"
      ~ teams       = [
            "P1V3E98",
          - "PZPZUBD",
        ]
        # (2 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

  # pagerduty_schedule.tridem_pl will be updated in-place
  ~ resource "pagerduty_schedule" "tridem_pl" {
        id          = "PI6AWLU"
        name        = "S4-SAM-FSM-Tridem_Schedule_PL"
      ~ teams       = [
            "P1V3E98",
          - "PZPZUBD",
        ]
        # (2 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 4 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

To see what breaks. Let me check.

Immediate replanning yields

Terraform will perform the following actions:

  # pagerduty_schedule.fallback will be updated in-place
  ~ resource "pagerduty_schedule" "fallback" {
        id          = "PCTVEUP"
        name        = "S4-SAM-FSM-Fallback_Schedule"
      ~ teams       = [
          - "P3Y7DTM",
          - "P1V3E98",
        ]
        # (2 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

So somehow the remote was recreated by PD under the hood as predicted.

Let me check the tridem_* resources

They look fine but lost one team association

I believe that team was once (in the past) part of the policy so it might be legit to remove.

So, next step: Add the two teams to my state.

Summary: Should be 2.0.0 I claim 🙉

gsreynolds commented 3 years ago

Hi @dominikmeyersap - as I understand the issue, the teams parameter in https://developer.pagerduty.com/api-reference/reference/REST/openapiv3.json/paths/~1schedules~1%7Bid%7D/put is partially read/write and partially read-only. To explain:

for team associations added automatically by PagerDuty (from user team membership on the schedule), these are read-only
for team associations added manually by users in the UI or via the API/Terraform

Where automatic user team membership and manual team associations line up, your Terraform will agree with the API and there will be no diff.

Where there are differences between the Team membership being applied by Terraform and PagerDuty's API:

If the team association does not exist in Terraform + does exist in PagerDuty + was added automatically = Terraform/API cannot remove these associations resulting in a permanent diff
If the team association does not exist in Terraform + does exist in PagerDuty + was added manually = Terraform will remove these associations
If the team association is in Terraform + does not exist in PagerDuty = Terraform will create these associations

As I see it, if Teams have been manually added in the UI or automatically by PagerDuty, then this state will need to be matched in the Terraform to avoid unexpectedly removing team associations or any permanent diffs.

ghost commented 3 years ago

Hi @gsreynolds,

thank you so much for the thorough analysis 🍻

I have to be honest, this explanations makes my brain smoke 🤣 Maybe I just do not understand the domain good enough.

Maybe a counter-question: Wouldn't it then make "more" sense to maintain a additionalTeams field? Because mixing the teams now kind of "broke" our TF state.

To my initial move:

If the team association does not exist in Terraform + does exist in PagerDuty + was added automatically = Terraform/API cannot remove these associations resulting in a permanent diff

This is my understanding why I advocate that 1.10.1 should be 2.0.0 as this IMO breaks backward compatibility? Or do I still get it wrong.

Anywho I do not really mind to add that code, but I still (after your explanations) have issues to follow it through on a code perspective. I mean I get what you are doing, I get what has been done. But I don't get why this did not lighten up some warning lights in PD as it will confuse users to have automatically and manually created entries mixed, some of them becoming read-only, some not 😏

ghost commented 3 years ago

Boop, need to make another PR because the teams reference self-updated 😢

NargiT commented 2 years ago

This is the reason we do not use Team because It's not terraform friendly and thus not gitops friendly.

PagerDuty / terraform-provider-pagerduty

1.10.1 should be 2.0.0 #374

Terraform Version

Affected Resource(s)

Terraform Configuration Files

Debug Output

Panic Output

Expected Behavior

Actual Behavior

Steps to Reproduce

Important Factoids

References