chore: Uses noOpCreate to avoid inconsistent result error and fix/enable tests in Ci #2290

EspenAlbert · 2024-05-21T08:33:45Z

Description

Enable data tests for mongodbatlas_federated_settings_org_config and refactored and run resource test locally
Enable test case with import+update+import test in CI for mongodbatlas_federated_settings_identity_provider
Remove migration tests since they were always skipped and not working on the import
Side note: I tried running these, but since the first step is import, the "default" approach won't work. I made some simple attempts, but I didn't find an "easy" way to do them.
Uses noOpCreate to avoid inconsistent result error (in both resources)

This resource is not meant to be imported see docs (will get changed when we implement OIDC fully)

Original error showing:

module.federated_vars[0].mongodbatlas_federated_settings_identity_provider.identity_provider: Creating...
╷
│ Error: Provider produced inconsistent result after apply
│
│ When applying changes to module.federated_vars[0].mongodbatlas_federated_settings_identity_provider.identity_provider, provider "provider[\"registry.terraform.io/mongodb/mongodbatlas\"]" produced an unexpected new value: Root object was present, but now absent.
│
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.

After refactoring:

│ Error: This resource must be imported
│
│   with module.federated_vars[0].mongodbatlas_federated_settings_identity_provider.identity_provider,
│   on modules/federated_vars/federated_vars.tf line 26, in resource "mongodbatlas_federated_settings_identity_provider" "identity_provider":
│   26: resource "mongodbatlas_federated_settings_identity_provider" "identity_provider" {

Link to any related issue(s): CLOUDP-248069

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. A migration guide must be created or updated if the new feature will go in a major version.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contributing guides
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.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

mongodbatlas_federated_settings_org_config test

go test -timeout 2400s -run ^TestAccFederatedSettingsOrg_basic$ github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedsettingsorgconfig -v

=== RUN   TestAccFederatedSettingsOrg_basic
=== PAUSE TestAccFederatedSettingsOrg_basic
=== CONT  TestAccFederatedSettingsOrg_basic
--- PASS: TestAccFederatedSettingsOrg_basic (11.90s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedsettingsorgconfig  13.028s

…ted_settings_connected_org

marcosuma · 2024-05-21T09:43:09Z

@EspenAlbert I am trying to understand if the two things are related with each other: noOp and setId. Are they? Do we have acceptance tests in CI that are testing this path?

EspenAlbert · 2024-05-21T10:29:16Z

@marcosuma we have acceptance test for:
mongodbatlas_federated_settings_identity_provider

But no acceptance test running in CI for mongodbatlas_federated_settings_org_config

setId must be called when a resource is created. This PR uses NoOp to be explicit about when the Create is attempted. I removed the if d.Id() == "" { and SetId("") since this code path was only hit during Create (Update and Import explicitly use SetId before calling the Read method)

Not sure if that answers your question.
WDYT? I can work on enabling the test in the CI or at least run in locally for mongodbatlas_federated_settings_org_config

marcosuma · 2024-05-22T05:28:02Z

@EspenAlbert it seems like those tests can't be enabled, because I see acc.SkipTestForCI(tb) // affects the org as a comment. We should check with @AgustinBettati and team to see if there is a chance we can figure a way to enable them.

For sure running those tests locally is a workaround for this PR, make sure that removing SetId("") does not change the behaviour or introduces a bug

EspenAlbert · 2024-05-23T07:14:41Z

@marcosuma I have been running the tests locally and found a few issues, I had a meeting with @AgustinBettati and we discussed how to test these.

I'm working on enabling some of these tests in the CI and having this PR ready for review.
Later, I believe a follow-up PR with full CRUD functionality and testing can be added for federated_settings_identity_provider using OIDC

marcosuma · 2024-05-23T09:55:23Z

Great, let's try to improve testing around this area. Just to confirm (I am still a bit unsure I have fully understood):

you are adding the NoOp because by simply removing the if Id() == "" then SetId("" wouldn't be enough. Can you confirm?
you have still not manage to test this correctly, right? I guess in this scenarios TDD can be handy

EspenAlbert · 2024-05-23T10:09:19Z

Great, let's try to improve testing around this area. Just to confirm (I am still a bit unsure I have fully understood):

you are adding the NoOp because by simply removing the if Id() == "" then SetId("" wouldn't be enough. Can you confirm?

Confirmed. After removing this check, it would fail on the read since Id() would be empty, and the read call would fail.

you have still not manage to test this correctly, right? I guess in this scenarios TDD can be handy

I have tested successfully 😄
Run in CI for the mongodbatlas_federated_settings_identity_provider

Local run for the TestAccFederatedSettingsOrg_basic:

go test -timeout 2400s -run ^TestAccFederatedSettingsOrg_basic$ github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedsettingsorgconfig -v

=== RUN   TestAccFederatedSettingsOrg_basic
=== PAUSE TestAccFederatedSettingsOrg_basic
=== CONT  TestAccFederatedSettingsOrg_basic
--- PASS: TestAccFederatedSettingsOrg_basic (11.90s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedsettingsorgconfig  13.028s

...vice/federatedsettingsidentityprovider/resource_federated_settings_identity_provider_test.go

marcosuma · 2024-05-24T07:18:35Z

...rnal/service/federatedsettingsorgconfig/data_source_federated_settings_connected_org_test.go

@@ -28,8 +26,7 @@ func TestAccFederatedSettingsOrgDS_basic(t *testing.T) {
 					resource.TestCheckResourceAttrSet(resourceName, "federation_settings_id"),
 					resource.TestCheckResourceAttrSet(resourceName, "role_mappings.#"),
 					resource.TestCheckResourceAttrSet(resourceName, "identity_provider_id"),
-					resource.TestCheckResourceAttrSet(resourceName, "org_id"),
-					resource.TestCheckResourceAttr(resourceName, "identity_provider_id", "0oad4fas87jL5Xnk1297"),


can't be known?

Replaced org_id with a more specific check (value of org_id ).
Removed identity_provider_id as it depends on the environment. Could add an env-var for the specific value but seems fragile and overkill 😅

marcosuma · 2024-05-24T07:19:17Z

LGTM! Left some minor comments

AgustinBettati

LGTM

internal/service/federatedsettingsorgconfig/resource_federated_settings_connected_org_test.go

AgustinBettati · 2024-05-24T07:40:50Z

...atedsettingsidentityprovider/resource_federated_settings_identity_provider_migration_test.go

-	"github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/mig"
-)
-
-func TestMigFederatedSettingsIdentityProviderRS_basic(t *testing.T) {


Would make sure once we implement full CRUD we add a migration test. cc: @oarbusi

internal/service/federatedsettingsorgconfig/resource_federated_settings_connected_org_test.go

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

refactor: use noOpCreate to avoid inconsistent result error

ede88cd

EspenAlbert changed the title ~~refactor: Uses noOpCreate to avoid inconsistent result error~~ chore: Uses noOpCreate to avoid inconsistent result error May 21, 2024

refactor: use noOpCreate to avoid inconsistent result error in federa…

d9287f0

…ted_settings_connected_org

EspenAlbert added 2 commits May 21, 2024 15:40

test: refactor test for mongodbatlas_federated_settings_org_config

48cc59d

test: cannot do a migration test when there is only an import step

6845bc8

EspenAlbert added 9 commits May 22, 2024 10:39

test: skip test in CI and add test for noOpCreate

8682b55

test: no reason to keep a test that will only be skipped

b8ffbc9

test: add test for createError

5dc00fa

test: renameTest

7c39d83

test: remove missleading steps

0c48f0c

test: remove missleading test steps

d244116

test: add import verify step after to ensure test works

795fa74

ci: re-enable test in CI and use static resource names

2c60224

test: fix DOMAIN_NOT_IN_FEDERATION_SETTINGS bug

3748ec2

EspenAlbert added 2 commits May 23, 2024 09:28

test: try adding domain since at least one domain must be active

7ebaa8a

test: change to a validated domain

c287674

EspenAlbert added 2 commits May 23, 2024 16:30

test: support reading MONGODB_ATLAS_FEDERATED_SETTINGS_ASSOCIATED_DOMAIN

b7a6a65

test: enable data tests for connected_org

2e00d90

EspenAlbert marked this pull request as ready for review May 24, 2024 06:49

EspenAlbert requested a review from a team as a code owner May 24, 2024 06:49

EspenAlbert changed the title ~~chore: Uses noOpCreate to avoid inconsistent result error~~ chore: Uses noOpCreate to avoid inconsistent result error and fix/enable tests in Ci May 24, 2024

github-actions bot added the bug label May 24, 2024

marcosuma reviewed May 24, 2024

View reviewed changes

...vice/federatedsettingsidentityprovider/resource_federated_settings_identity_provider_test.go Outdated Show resolved Hide resolved

marcosuma reviewed May 24, 2024

View reviewed changes

marcosuma approved these changes May 24, 2024

View reviewed changes

AgustinBettati approved these changes May 24, 2024

View reviewed changes

lantoli reviewed May 24, 2024

View reviewed changes

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

lantoli approved these changes May 24, 2024

View reviewed changes

EspenAlbert added 2 commits May 24, 2024 15:37

use spaces instead of tabs

0bdf6d5

add PR comments/suggestions

b4964e6

EspenAlbert merged commit 2b2e276 into master May 24, 2024
32 checks passed

EspenAlbert deleted the CLOUDP-248069_set_id_condition_federated_settings_identity_provider branch May 24, 2024 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Uses noOpCreate to avoid inconsistent result error and fix/enable tests in Ci #2290

chore: Uses noOpCreate to avoid inconsistent result error and fix/enable tests in Ci #2290

EspenAlbert commented May 21, 2024 •

edited

marcosuma commented May 21, 2024

EspenAlbert commented May 21, 2024

marcosuma commented May 22, 2024

EspenAlbert commented May 23, 2024 •

edited

marcosuma commented May 23, 2024

EspenAlbert commented May 23, 2024 •

edited

marcosuma May 24, 2024

EspenAlbert May 24, 2024

marcosuma commented May 24, 2024

AgustinBettati left a comment

AgustinBettati May 24, 2024

chore: Uses noOpCreate to avoid inconsistent result error and fix/enable tests in Ci #2290

chore: Uses noOpCreate to avoid inconsistent result error and fix/enable tests in Ci #2290

Conversation

EspenAlbert commented May 21, 2024 • edited

Description

Type of change:

Required Checklist:

Further comments

marcosuma commented May 21, 2024

EspenAlbert commented May 21, 2024

marcosuma commented May 22, 2024

EspenAlbert commented May 23, 2024 • edited

marcosuma commented May 23, 2024

EspenAlbert commented May 23, 2024 • edited

marcosuma May 24, 2024

Choose a reason for hiding this comment

EspenAlbert May 24, 2024

Choose a reason for hiding this comment

marcosuma commented May 24, 2024

AgustinBettati left a comment

Choose a reason for hiding this comment

AgustinBettati May 24, 2024

Choose a reason for hiding this comment

EspenAlbert commented May 21, 2024 •

edited

EspenAlbert commented May 23, 2024 •

edited

EspenAlbert commented May 23, 2024 •

edited