use openAppSecrets instead of list+Open secret in DataSourceVaultSecr…

[hghaf099]

🛠️ Description

Addresses: https://hashicorp.atlassian.net/browse/VAULT-26495
We currently call ListAppSecrets, iterate over each secret and individually invoke OpenAppSecret here. This is both inefficient for us and causes additional API usage for the clients.

I think switching to OpenAppSecrets for this data source would be best.

🏗️ Acceptance tests

• Are there any feature flags that are required to use this functionality?
• Have you added an acceptance test for the functionality being added?
• Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAcc_dataSourceVaultSecretsApp'

...

=== RUN   TestAcc_dataSourceVaultSecretsAppMigration
--- PASS: TestAcc_dataSourceVaultSecretsAppMigration (12.94s)
=== RUN   TestAcc_dataSourceVaultSecretsApp
    data_source_vault_secrets_app_test.go:97: secrets:  secret_one  =  hey, this is version 1!
    data_source_vault_secrets_app_test.go:97: secrets:  secret_two  =  hey, this is version 2!
--- PASS: TestAcc_dataSourceVaultSecretsApp (6.39s)
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider/vaultsecrets      20.318s
testing: warning: no tests to run
PASS

To test this, I also created the following helper function:

func openTestAppSecret(t *testing.T, appName string) {
	t.Helper()

	client := acctest.HCPClients(t)

	loc := &sharedmodels.HashicorpCloudLocationLocation{
		OrganizationID: client.Config.OrganizationID,
		ProjectID:      client.Config.ProjectID,
	}

	secrets, err := clients.OpenVaultSecretsAppSecrets(context.Background(), client, loc, appName)
	if err != nil {
		t.Fatal(err)
	}
	if len(secrets) != 2 {
		t.Fatal("failed to read the secrets")
	}
	for _, s := range secrets {
		t.Log("secrets: ", s.Name, " = ", s.Version.Value)
	}

}

Added the above function to TestAcc_dataSourceVaultSecretsApp test on line 97 (which is after the test creates two secrets). The test passes and generates the following DD log:

[codergs]

Great, this looks good me!

[codergs]

nit: I know this bloats code, but to provide an abstraction with type casting to all known vault secrets input/output types seems poor for readability to me personally, but we can always consider doing that. also, should be a separate ticket.

[hghaf099]

I agree with the type casting, which I think I tried to avoid it. The type is being examined in the body of this function. But, then the generic part of retry logic is refactored.
I considered working on the refactoring part in another ticket, but, the change I believe is small enough to include it here.

[codergs]

Not seeing the test openTestAppSecret you mentioned in the comments.

[hghaf099]

Not seeing the test openTestAppSecret you mentioned in the comments.

I provided a snippet of that. I just used that for local testing.

[codergs]

Not seeing the test openTestAppSecret you mentioned in the comments.

I provided a snippet of that. I just used that for local testing.

If you want this test persisted, then we should be able to extend our acceptance tests easily.

[hghaf099]

Not seeing the test openTestAppSecret you mentioned in the comments.

I provided a snippet of that. I just used that for local testing.

If you want this test persisted, then we should be able to extend our acceptance tests easily.

So, the TestAcc_dataSourceVaultSecretsApp test actually practices the new code path. So, this data source runs the new code for which I have posted the DD log screen shot. The code snippet I provided was just for own sake as part of double assurance. But, that is really not necessary in my view.

[codergs]

Not seeing the test openTestAppSecret you mentioned in the comments.

I provided a snippet of that. I just used that for local testing.

If you want this test persisted, then we should be able to extend our acceptance tests easily.

So, the TestAcc_dataSourceVaultSecretsApp test actually practices the new code path. So, this data source runs the new code for which I have posted the DD log screen shot. The code snippet I provided was just for own sake as part of double assurance. But, that is really not necessary in my view.

Ahh, understood. Existing test is fine then.

[codergs]

Can't approve as it's in draft.

[manish-hashicorp]

Rubber stamping

[nywilken]

LGTM