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

EspenAlbert commented 1 month ago

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:

[x] I have signed the MongoDB CLA
[x] I have read the contributing guides
[x] I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
[x] I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
[x] I have added any necessary documentation (if appropriate)
[x] I have run make fmt and formatted my code
[x] 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.
[x] 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

marcosuma commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

marcosuma commented 1 month ago

LGTM! Left some minor comments

mongodb / terraform-provider-mongodbatlas