chore: Migrates federated_settings_identity_provider to new auto-generated SDK #1808

oarbusi · 2024-01-04T14:12:53Z

Description

migrates federated_settings_identity_provider to new auto-generated SDK
Because a breaking change in the latest version of the federated settings API, some changes needed to be made to grant users a grace period to avoid breaking changes in next release:

Allow both formats for identityProviderId.
Put a warning when using the old ID, and linking to the migration guide. In the future only new format for identityProviderId will be allowed.

Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-220807

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

oarbusi · 2024-01-04T14:16:31Z

...rnal/service/federatedsettingsidentityprovider/model_federated_settings_identity_provider.go

moved all flatteners to this new file. Some of the flatteners are also used in federated_settings_org_config so I did not change those to new SDK. Instead, created new methods for the ones used by new SDK.

oarbusi · 2024-01-04T15:34:34Z

...rvice/federatedsettingsidentityprovider/data_source_federated_settings_identity_providers.go

 	if !federationSettingsIDOk {
 		return diag.FromErr(errors.New("federation_settings_id must be configured"))
 	}
+	// There is no available paginated method


as of now there is no way to paginate results in the new SDK

Although the old SDK defined list options they were not used as the options are not present in the v1 API: https://www.mongodb.com/docs/atlas/reference/api-resources-spec/v1/#tag/Federated-Authentication/operation/listIdentityProviders

Given that page_num and items_per_page are not being used, should we create a followup ticket to deprecate them?

Makes sense to me. I can create the ticket
edit: Created the ticket: CLOUDP-221367

marcosuma · 2024-01-05T06:54:13Z

LGTM! Good to have a second look for this change

...rvice/federatedsettingsidentityprovider/data_source_federated_settings_identity_providers.go

AgustinBettati · 2024-01-05T09:05:06Z

...ervice/federatedsettingsidentityprovider/data_source_federated_settings_identity_provider.go

@@ -201,7 +201,7 @@ func DataSource() *schema.Resource {
 }
 func dataSourceMongoDBAtlasFederatedSettingsIdentityProviderRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
 	// Get client connection.
-	conn := meta.(*config.MongoDBClient).Atlas
+	connV2 := meta.(*config.MongoDBClient).AtlasV2


As acceptance test for the resource and data sources are not being run in the CI, wanted to confirm if you were able to run them locally.

I have run all acceptance tests locally successfully

...rnal/service/federatedsettingsidentityprovider/model_federated_settings_identity_provider.go

maastha · 2024-01-05T10:35:39Z

...vice/federatedsettingsidentityprovider/resource_federated_settings_identity_provider_test.go

@@ -11,13 +11,13 @@ import (
 	"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/conversion"
 	"github.com/mongodb/terraform-provider-mongodbatlas/internal/config"
 	"github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/acc"
-	matlas "go.mongodb.org/atlas/mongodbatlas"
+	"go.mongodb.org/atlas-sdk/v20231115002/admin"
 )

 func TestAccFedRSFederatedSettingsIdentityProvider_basic(t *testing.T) {


When do you plan to add migration tests? If planning to merge this to master right away we should add some as part of this PR and update in subsequent PRs

...rnal/service/federatedsettingsidentityprovider/model_federated_settings_identity_provider.go

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go

internal/config/client.go

oarbusi · 2024-01-11T11:56:49Z

website/docs/d/federated_settings_identity_provider.html.markdown

-* `identity_provider_id` - (Required) Unique 20-hexadecimal digit string that identifies the IdP. 
+* `identity_provider_id` - (Required) Unique 20-hexadecimal or 24-hexadecimal digit string that identifies the IdP. 
+
+**WARNING:** Starting at terraform provider 1.16.0 the allowed format for `identity_provider_id` will only be 24-hexadecimal digit string. See more [here](../guides/1.15.0-upgrade-guide.html.markdown)


will create a draft of the upgrade guide in a follow up PR

@Zuhairahmed your input for these changes in documentation would be appreciated.

oarbusi · 2024-01-11T11:56:55Z

website/docs/r/federated_settings_identity_provider.html.markdown

@@ -58,4 +59,6 @@ Identity Provider **must** be imported before using federation_settings_id-idp_i
 $ terraform import mongodbatlas_federated_settings_identity_provider.identity_provider 6287a663c660f52b1c441c6c-0oad4fas87jL5Xnk1297
 ```

+**WARNING:** Starting from terraform provider version 1.16.0, to import Identity Provider, `id` will have to be used instead of `okta_idp_id`. See more [here](../guides/1.15.0-upgrade-guide.html.markdown)


will create a draft of the upgrade guide in a follow up PR

oarbusi · 2024-01-11T14:56:22Z

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go

+		Severity: diag.Warning,
+		Summary:  "Warning: deprecated identity provider id",
+		Detail: "Identity provider id value used will be deprecated. Please start using the new value.\n" +
+			" Follow instructions here: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/guides/1.15.0-upgrade-guide",


@Zuhairahmed your input would be appreciated here

AgustinBettati · 2024-01-11T17:02:46Z

website/docs/r/federated_settings_identity_provider.html.markdown

@@ -49,6 +49,7 @@ In addition to all arguments above, the following attributes are exported:
 ### FederatedSettingsIdentityProvider

 * `okta_idp_id` - Unique 20-hexadecimal digit string that identifies the IdP.
+* `id` - Unique 24-hexadecimal digit string that identifies the IdP.


Is this accurate? From my understanding the id of the resource is not modified, and continues to be de the encoded value of the federation_settings_id and okta_idp_id.

you are right, the terraform state id continues to be de the encoded value of the federation_settings_id and okta_idp_id. But once users start using the new id, terraform state id will be federation_settings_id and id encoded

I do agree that if the users does an import command, the id value of the resource with be the encoded value of federation_settings_id + identity provider id (both new and old format will be supported). However, my question is if this documentation of id which was added is accurate, because the id value will still be an encoded value which I doubt users will be able to use.

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go

AgustinBettati · 2024-01-11T17:20:41Z

website/docs/r/federated_settings_identity_provider.html.markdown

@@ -58,4 +59,6 @@ Identity Provider **must** be imported before using federation_settings_id-idp_i
 $ terraform import mongodbatlas_federated_settings_identity_provider.identity_provider 6287a663c660f52b1c441c6c-0oad4fas87jL5Xnk1297
 ```

+**WARNING:** Starting from terraform provider version 1.16.0, to import Identity Provider, `id` will have to be used instead of `okta_idp_id`. See more [here](../guides/1.15.0-upgrade-guide.html.markdown)


I believe it might be hard for users to understand what id is, might be better to rephrase to 24-hexadecimal digit string that identifies the IdP

…derated_settings_identity_provider.go Co-authored-by: Agustin Bettati <[email protected]>

oarbusi added 3 commits January 4, 2024 14:11

use new SDK in resource

7d636f4

use new SDK in resource tests

b38a5d5

migrate singular and plural data source to new SDK

212b38e

oarbusi commented Jan 4, 2024

View reviewed changes

oarbusi changed the title ~~chore: migrates federated_settings_identity_provider to new auto-generated SDK~~ chore: Migrates federated_settings_identity_provider to new auto-generated SDK Jan 4, 2024

oarbusi commented Jan 4, 2024

View reviewed changes

oarbusi marked this pull request as ready for review January 4, 2024 15:34

oarbusi requested a review from a team as a code owner January 4, 2024 15:34

marcosuma approved these changes Jan 5, 2024

View reviewed changes

AgustinBettati reviewed Jan 5, 2024

View reviewed changes

oarbusi added 2 commits January 5, 2024 10:28

change pointer access to getter

d261c2a

use TimePtrToStringPtr

91f9b28

maastha reviewed Jan 5, 2024

View reviewed changes

lantoli reviewed Jan 10, 2024

View reviewed changes

...rnal/service/federatedsettingsidentityprovider/model_federated_settings_identity_provider.go Show resolved Hide resolved

lantoli approved these changes Jan 10, 2024

View reviewed changes

oarbusi added 5 commits January 10, 2024 18:15

wip

037a98e

wip2

addd42d

use both latest SDK and previous version, add warning message

2d7416d

add ID and modify link

145a738

remove mig test

d67946f

lantoli reviewed Jan 11, 2024

View reviewed changes

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go Outdated Show resolved Hide resolved

lantoli reviewed Jan 11, 2024

View reviewed changes

internal/config/client.go Outdated Show resolved Hide resolved

final changes

c91401e

oarbusi commented Jan 11, 2024

View reviewed changes

oarbusi requested a review from AgustinBettati January 11, 2024 11:57

oarbusi added 3 commits January 11, 2024 13:00

add comment in OldAtlasV2

3c00366

lint

5f3f7a8

remove alias

be25056

oarbusi commented Jan 11, 2024

View reviewed changes

AgustinBettati reviewed Jan 11, 2024

View reviewed changes

oarbusi and others added 2 commits January 12, 2024 09:08

Update internal/service/federatedsettingsidentityprovider/resource_fe…

1e426bf

…derated_settings_identity_provider.go Co-authored-by: Agustin Bettati <[email protected]>

clarify doc

f0ad7d5

oarbusi merged commit 9cf3f9f into master Jan 12, 2024

oarbusi deleted the CLOUDP-220807 branch January 12, 2024 08:16

oarbusi restored the CLOUDP-220807 branch January 18, 2024 10:31

oarbusi deleted the CLOUDP-220807 branch January 18, 2024 10:33

oarbusi mentioned this pull request Jan 18, 2024

chore: Use new attributes from new SDK in Federated Auth #1842

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Migrates federated_settings_identity_provider to new auto-generated SDK #1808

chore: Migrates federated_settings_identity_provider to new auto-generated SDK #1808

oarbusi commented Jan 4, 2024 •

edited

Loading

oarbusi Jan 4, 2024 •

edited

Loading

oarbusi Jan 4, 2024

AgustinBettati Jan 5, 2024

oarbusi Jan 5, 2024 •

edited

Loading

marcosuma commented Jan 5, 2024

AgustinBettati Jan 5, 2024

oarbusi Jan 11, 2024

maastha Jan 5, 2024 •

edited

Loading

oarbusi Jan 11, 2024

oarbusi Jan 11, 2024

oarbusi Jan 11, 2024

oarbusi Jan 11, 2024

AgustinBettati Jan 11, 2024

oarbusi Jan 12, 2024

AgustinBettati Jan 12, 2024 •

edited

Loading

AgustinBettati Jan 11, 2024

oarbusi Jan 12, 2024

chore: Migrates federated_settings_identity_provider to new auto-generated SDK #1808

chore: Migrates federated_settings_identity_provider to new auto-generated SDK #1808

Conversation

oarbusi commented Jan 4, 2024 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

oarbusi Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oarbusi Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

marcosuma commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maastha Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgustinBettati Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oarbusi commented Jan 4, 2024 •

edited

Loading

oarbusi Jan 4, 2024 •

edited

Loading

oarbusi Jan 5, 2024 •

edited

Loading

maastha Jan 5, 2024 •

edited

Loading

AgustinBettati Jan 12, 2024 •

edited

Loading