Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle external change of secret type #3141

Merged
merged 17 commits into from
Oct 24, 2024

Conversation

sfc-gh-fbudzynski
Copy link
Collaborator

@sfc-gh-fbudzynski sfc-gh-fbudzynski commented Oct 17, 2024

Changes

  • Added secret_type computed filed to secret resources
  • Added CustomDiff for handling external change of secret_type
  • Refactored show_and_describe_handlers for flat describe_output and show_output
  • Tested external change of secret type for each resource

Test Plan

  • acceptance tests

References

Copy link

Integration tests failure for 510df15a291f193bd9733d423af438b312e12ef9

Copy link

Integration tests failure for 7d90d7b0db639fdcbea2fdaf6f212aefd883b9f2

Copy link

Integration tests failure for dd38109b6b71fb199b4eb6cc0b03d03cb437c1a1

@sfc-gh-fbudzynski sfc-gh-fbudzynski marked this pull request as ready for review October 18, 2024 14:37
@sfc-gh-fbudzynski sfc-gh-fbudzynski changed the title fix: handle secret type external change fix: handle external change of secret type Oct 18, 2024
Copy link

Integration tests failure for 90721b42eaf1fb9604965548f225d113cca01334

HasOauthRefreshTokenExpiryTimeString(refreshTokenExpiryDateTime),
assert.Check(resource.TestCheckResourceAttrSet(secretModel.ResourceReference(), "describe_output.0.oauth_refresh_token_expiry_time")),
),
),
},
{
RefreshState: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this kind of test case for the first time, could you briefly describe what it does/tests ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When discussing the refresh_token_expiry_time field with @sfc-gh-jmichalak we wanted to check if there will be a perma diff or not when invoking Read on that resource in order to be sure if we needed a diff suppress.

There was no perma diff because this filed is not being overwritten by value in describe, it is handled by using setStateToValuesFromConfig

withExternalChangesMarking and handleExternalValueChangesToObjectInDescribe is being used only to detect the external change on that field.

Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diffs after read are usually implicitly caught by the testing framework, but If we want to check that explicitly let's make a comment above this test case to know what are we actually asserting.

Copy link

Integration tests failure for 49dc3b8dfa8ac335618d359bb46d2f65826d2d5f

Copy link

Integration tests failure for b05ad4d69cf3c449cd2a7000983b83087a4b8ff3

Copy link

Integration tests failure for 36c16a0ac744681341ae838981058762cbaefba7

Copy link

Integration tests failure for 97e7aae5717601a8e91cea9934daa667b5b599b0

pkg/sdk/secrets_def.go Outdated Show resolved Hide resolved
pkg/sdk/secrets_def.go Outdated Show resolved Hide resolved
pkg/resources/secret_with_basic_authentication.go Outdated Show resolved Hide resolved
pkg/resources/custom_diffs.go Outdated Show resolved Hide resolved
pkg/resources/custom_diffs.go Outdated Show resolved Hide resolved
pkg/resources/custom_diffs.go Show resolved Hide resolved
pkg/resources/secret_common.go Outdated Show resolved Hide resolved
pkg/resources/show_and_describe_handlers.go Show resolved Hide resolved
pkg/resources/custom_diffs.go Outdated Show resolved Hide resolved
Copy link

Integration tests success for cf8b7e43e3f574144028de9cd2a40f079ce012a3

SecretTypeGenericString = "GENERIC_STRING"
OAuth2ClientCredentialsFlow OauthSecretType = "CLIENT_CREDENTIALS" // #nosec G101
OAuth2AuthorizationCodeGrantFlow OauthSecretType = "AUTHORIZATION_CODE_GRANT" // #nosec G101
SecretTypePassword SecretType = "PASSWORD"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't like that this is one enum here. Two different use cases are mixed here: SQL TYPE value and creation variants. I'm okay with merging this but let's talk later how we can improve this.

Copy link

Integration tests failure for 9cbbca0fd625f337d71b7c3e86ff5df60937bf25

Copy link

Integration tests failure for 55ee85b119f16006ad0b625fa9f22f5086870a0d

@sfc-gh-fbudzynski sfc-gh-fbudzynski merged commit 649b839 into main Oct 24, 2024
8 of 9 checks passed
@sfc-gh-fbudzynski sfc-gh-fbudzynski deleted the secret-type-external-change branch October 24, 2024 08:31
sfc-gh-jmichalak pushed a commit that referenced this pull request Nov 8, 2024
##
[0.98.0](v0.97.0...v0.98.0)
(2024-11-08)

Feature scope readiness for V1:
[link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD)
([Roadmap
reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#wrap-up-the-functional-scope)).
:exclamation: Migration guide: [v0.97.0 ->
v0.98.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0970--v0980)

### 🎉 What's new
- New resources:
- authentication_policy
([#3098](#3098)),
references
[#2880](#2880)
- external_volume
([#3106](#3106)),
partially references
[#2980](#2980)
- stream_on_directory_table
([#3129](#3129))
- stream_on_view
([#3150](#3150))
- primary_connection, secondary_connection
([#3162](#3162))
- secret_with_basic_authentication, secret_with_generic_string,
secret_with_oauth_authorization_code_grant,
secret_with_oauth_client_credentials
([#3110](#3110)),
([#3141](#3141))
- New data sources:
- connections
([#3155](#3155)),
([#3173](#3173))
- secrets
([#3131](#3131))
- Reworked:
- provider configuration hierarchy
([#3166](#3166)),
references
[#1881](#1881),
[#2145](#2145),
[#2925](#2925),
[#2983](#2983),
[#3104](#3104)
- provider configuration fields
([#3152](#3152))
streams data source
([#3151](#3151))
- SDK upgrades:
- Upgrade tag SDK
([#3126](#3126))
- Recreate streams when they are stale
([#3129](#3129))
### 🔧  Misc
- Add object renaming research summary
([#3172](#3172))
- Test support for object renaming
([#3130](#3130)),
([#3147](#3147)),
([#3154](#3154))
- Add tests to issue
[#3117](#3117)
([#3133](#3133))
- New roadmap entry
([#3158](#3158))
- Test more authentication methods
([#3178](#3178))
- Minor fixes
([#3174](#3174))
### 🐛  Bug fixes
- Apply various fixes
([#3176](#3176)),
this addresses BCR 2024_08, references
[#2717](#2717),
[#3005](#3005),
[#3125](#3125),
[#3127](#3127),
[#3153](#3153)
- Connection and secret data sources tests
([#3177](#3177))
- Fix grant import docs
([#3183](#3183)),
resolves
[#3179](https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/3179)
- Fix user resource import
([#3181](#3181))
- Handle external type changes in stream resources
([#3164](#3164))
- Do not use OR REPLACE on initial creation in resources with
copy_grants
([#3129](#3129))
- Address issue
[#2201](#2201)
by introducing new stream resources

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants