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

Only manage team memberships if they were defined before in the config #162

Merged
2 commits merged into from
Dec 4, 2023

Conversation

ghost
Copy link

@ghost ghost commented Nov 29, 2023

Issues

Some customers are defining Teams in terraform, but are not defining team memberships in their configurations (*.tf files) using member {} blocks. They are unable to run this provider because doing so results in all members being unassigned since they are not defined in terraform.

One option for them is to export their members into their terraform config using the CLI.

The other option is for the terraform provider to only touch Team Memberships if they are defined or were previously defined at any point before in the configuration.

Related, export members from web into terraform using CLI: OpsLevel/cli#206

Related, ability to use member {} blocks in terraform: #144

Changelog

  • Change read function on Team to look at memberships only if they are being set right now or were set in the past.
  • Make a changie entry

Tophatting

Create a new team without any managed members, make sure terraform doesn't try to unassign members on updates

Create the team

  # opslevel_team.test_team will be created
  + resource "opslevel_team" "test_team" {
      + aliases          = [
          + "special_team",
        ]
      + id               = (known after apply)
      + last_updated     = (known after apply)
      + name             = "special team"
      + responsibilities = "hello world"
    }

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

On the website, add 2 contributors and 1 manager.

image

Run the plan. Expected is to see no changes, since no member {} is defined in the config.

opslevel_team.test_team: Refreshing state... [id=Z2lkOi8vb3BzbGV2ZWwvVGVhbS8xMzI5OA]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.

Try again by running an update, but without managing team memberships.

  # opslevel_team.test_team will be updated in-place
  ~ resource "opslevel_team" "test_team" {
        id               = "Z2lkOi8vb3BzbGV2ZWwvVGVhbS8xMzI5OA"
        name             = "special team"
      ~ responsibilities = "hello world" -> "i changed this"
        # (1 unchanged attribute hidden)
    }

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

This shows that you can manage a team that has no member {} blocks without the provider trying to unassign everyone.

Add a single member {} block. This should cause terraform to reconcile team memberships, removing the 3 current members with Doug since it is now managing members too.

  # opslevel_team.test_team will be updated in-place
  ~ resource "opslevel_team" "test_team" {
        id               = "Z2lkOi8vb3BzbGV2ZWwvVGVhbS8xMzI5OQ"
        name             = "special team"
        # (3 unchanged attributes hidden)

      ~ member {
          ~ email = "[email protected]" -> "[email protected]"
            # (1 unchanged attribute hidden)
        }
      - member {
          - email = "[email protected]" -> null
          - role  = "contributor" -> null
        }
      - member {
          - email = "[email protected]" -> null
          - role  = "contributor" -> null
        }
    }

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

Remove the last member.

  # opslevel_team.test_team will be updated in-place
  ~ resource "opslevel_team" "test_team" {
        id               = "Z2lkOi8vb3BzbGV2ZWwvVGVhbS8xMzI5OQ"
        name             = "special team"
        # (3 unchanged attributes hidden)

      - member {
          - email = "[email protected]" -> null
          - role  = "manager" -> null
        }
    }

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

Now from here, we are in the state where the configuration has no members, and the object itself has no members. BUT, GetOk() returns true if something was ever set in the configuration. So I expect that it will try to reconcile team memberships if I add some in the website.

image
  # opslevel_team.test_team will be updated in-place
  ~ resource "opslevel_team" "test_team" {
        id               = "Z2lkOi8vb3BzbGV2ZWwvVGVhbS8xMzI5OQ"
        name             = "special team"
        # (3 unchanged attributes hidden)

      - member {
          - email = "[email protected]" -> null
          - role  = "manager" -> null
        }
    }

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

@ghost ghost self-assigned this Nov 29, 2023
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (ce26fff) 44.85% compared to head (e933c76) 44.85%.

Files Patch % Lines
opslevel/resource_opslevel_team.go 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
- Coverage   44.85%   44.85%   -0.01%     
==========================================
  Files          70       70              
  Lines        5642     5643       +1     
==========================================
  Hits         2531     2531              
- Misses       3109     3110       +1     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghost ghost force-pushed the team-members-read branch from 4a801e8 to d87de6b Compare November 29, 2023 19:15
@ghost ghost marked this pull request as ready for review November 29, 2023 19:22
@ghost ghost requested review from rocktavious and davidbloss November 29, 2023 19:22
// in their config, and they cannot use terraform to manage
// teams because of it without either adding the members into
// the config or unassigning all the members (unwanted)
if members, ok := d.GetOk("member"); members != nil || ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the members != nil || part of this check?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, because if that part is not present it won't detect when member is defined for the first time in a configuration.

The first result will not necessarilly be nil if the value doesn't exist. The second result should be checked to determine this information.

https://pkg.go.dev/github.com/hashicorp/terraform/helper/schema#ResourceData.GetOk

@ghost ghost merged commit e1f983d into main Dec 4, 2023
5 of 6 checks passed
@ghost ghost deleted the team-members-read branch December 4, 2023 14:39
@ghost
Copy link
Author

ghost commented Dec 15, 2023

Warning: I think we have a regression with this bug.

Tophatting by compiling the latest main locally.

Create the team

$ cat main.tf
resource "opslevel_team" "regteam" {
  aliases = []
  name = "regression team"
}

$ terraform apply

Add a manager and a contributor on web

Don't change the terraform config at all, just hit $ terraform apply again. We shouldn't see this output, terraform provider should detect no changes.

  # opslevel_team.regteam will be updated in-place
  ~ resource "opslevel_team" "regteam" {
        id               = "Z2lkOi8vb3BzbGV2ZWwvVGVhbS8xNDIwMQ"
        name             = "regression team"
      - responsibilities = "so sad :(((((((((((" -> null
        # (1 unchanged attribute hidden)

      - member {
          - email = "[email protected]" -> null
          - role  = "manager" -> null
        }
      - member {
          - email = "[email protected]" -> null
          - role  = "contributor" -> null
        }
    }

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

This pull request was closed.
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