terraform-provider-okta icon indicating copy to clipboard operation
terraform-provider-okta copied to clipboard

Unclear documentation for skip_groups and lifecycle policies and group_assignments

Open fatbasstard opened this issue 1 year ago • 3 comments

Hi,

There is some unclarity in the documentation regarding the use of skip_groups and the lifecycle policy change.

We've got the following deprecation warning on an okta_app_oauth now:

│ Warning: Deprecated attribute
│ 
│   on .terraform/modules/xxx/auth.tf line 72, in resource "okta_app_oauth" "default":
│   72:     ignore_changes = [users, **groups**]

Since we're using okta_app_group_assignment I've replaced this part with the skip_groups and removed the lifecycle policy (because deprecated).

BUT:

The okta_app_group_assignment resource documentation still mentions this in the

When using this resource in conjunction with other application resources (e.g. okta_app_oauth)
it is advisable to add the following lifecycle argument to the associated app_* 
resources to prevent the groups being unassigned on subsequent runs:

resource "okta_app_oauth" "app" {
  //...
  lifecycle {
     ignore_changes = [groups]
  }
}

Which is confusing since the groups attribute is deprecated.

Also. in the okta_app_oauth resource I only get a warning about the groups being deprecated. There is no mention of the users while that is also documented as deprecated

Question:

What is the right approach? Is skip_groups enough to be implemented and can the lifecycle be omitted? (so documentation is off), and does the same apply to users? (so the deprecation warning is just missing)

fatbasstard avatar Aug 17 '22 11:08 fatbasstard

Thanks @fatbasstard , I'll call this a documentation bug.

I can see how groups are marked deprecated in the schema, the Terraform runtime uses that to generate the warning: https://github.com/okta/terraform-provider-okta/blob/master/okta/app.go#L68-L74 And I can see how users are marked deprecated https://github.com/okta/terraform-provider-okta/blob/master/okta/app.go#L61-L67 So maybe that is a bug/inconsistency that the Terraform runtime doesn't warn on all deprecated attributes it encounters ( perhaps just the first?).

I'd say, experiment if you can, with dropping lifecycle ignore changes on users and groups in okta_app_oauth and switch to using skip users and skip groups. Let me know if that works.

Also, the bug is we need to correct that documentation about suggesting utilizing any deprecated properties in the examples.

monde avatar Aug 17 '22 19:08 monde

Refactoring:

resource "okta_group" "xxx" {
  name        = "xxx"
  description = "xxx"

  lifecycle {
    ignore_changes = [users]
  }
}

into

resource "okta_group" "xxx" {
  name        = "xxx"
  description = "xxx"
  skip_users  = true

works. For both the okta_groups as the Okta apps (like okta_app_saml and okta_app_oauth ). In combination with the group assignments that is.

I think the only thing that needs to be changed is replace the lifecycle policy with the skip_ feature in the documentation.

fatbasstard avatar Aug 22 '22 11:08 fatbasstard

Okta internal reference https://oktainc.atlassian.net/browse/OKTA-526329

monde avatar Aug 22 '22 16:08 monde

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 5 days

github-actions[bot] avatar Oct 22 '22 00:10 github-actions[bot]

@fatbasstard can this issue be closed?

monde avatar Oct 24 '22 18:10 monde

Before closing this issue, can somebody please correct the necessary documentation (i.e. it is still referencing lifecycle usage, for example, https://registry.terraform.io/providers/okta/okta/latest/docs/resources/app_group_assignment and https://registry.terraform.io/providers/okta/okta/latest/docs/resources/app_group_assignments). Additionally, it would be helpful to update (reopen) https://github.com/okta/terraform-provider-okta/issues/1177 as the "lifecycle" confusion is spreading elsewhere (i.e. use "skip_groups" instead?).

johnnymongiat avatar Nov 02 '22 15:11 johnnymongiat