-
Notifications
You must be signed in to change notification settings - Fork 430
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: account for null comment #3417
Conversation
Integration tests cancelled for 5e6c3196b423bf988658052571594807b55b6de4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a test in resources/manual_tests
?
Integration tests cancelled for 9d25771071bd57ddc0b121b13973db77ace9c782 |
9d25771
to
d3ffe6e
Compare
pkg/resources/manual_tests/handling_account_null_comment/main.tf
Outdated
Show resolved
Hide resolved
pkg/resources/manual_tests/handling_account_null_comment/handling_account_null_comment.md
Show resolved
Hide resolved
pkg/resources/manual_tests/handling_account_null_comment/handling_account_null_comment.md
Outdated
Show resolved
Hide resolved
Integration tests failure for d3ffe6e8c5f9300a9e7078124f2756bfbbdeca51 |
if err = handleExternalChangesToObjectInShow(d, | ||
outputMapping{"edition", "edition", *account.Edition, *account.Edition, nil}, | ||
outputMapping{"is_org_admin", "is_org_admin", *account.IsOrgAdmin, booleanStringFromBool(*account.IsOrgAdmin), nil}, | ||
outputMapping{"region_group", "region_group", regionGroup, regionGroup, nil}, | ||
outputMapping{"snowflake_region", "region", account.SnowflakeRegion, account.SnowflakeRegion, nil}, | ||
outputMapping{"comment", "comment", *account.Comment, *account.Comment, nil}, | ||
outputMapping{"comment", "comment", comment, comment, nil}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is funny TBH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that we should make some adjustments to handle null pointers inside handleExternalChangesToObjectInShow
instead of defining non-pointer values before the mappings. It would simplify the codebase for some of the resources.
pkg/resources/manual_tests/handling_account_null_comment/handling_account_null_comment.md
Outdated
Show resolved
Hide resolved
Integration tests failure for 0a58799931eef0185de9e720749f5911b423a72c |
🤖 I have created a release *beep* *boop* --- ## [1.0.4](v1.0.3...v1.0.4) (2025-02-26) ### 🔧 **Misc** * Add GA scope to the roadmap ([#3385](#3385)) ([9be2196](9be2196)) * Adjust saml2_integration documentation ([#3415](#3415)) ([b8c127d](b8c127d)) * Bump Go version ([#3408](#3408)) ([902670e](902670e)) * Bump provider version ([#3419](#3419)) ([552e44b](552e44b)) * Remove hardcoded values from documentation ([#3381](#3381)) ([cb1d6e2](cb1d6e2)) * Update user docs ([#3416](#3416)) ([9d5224f](9d5224f)), closes [#3404](#3404) * Upgrade deps ([#3389](#3389)) ([086bf15](086bf15)) ### 🐛 **Bug fixes:** * account for null comment ([#3417](#3417)) ([2f7f80f](2f7f80f)) * external function varchar return type validation ([#3400](#3400)) ([abf5883](abf5883)) * Fix managed account and adjust account ticket numbers ([#3395](#3395)) ([a7cb5cb](a7cb5cb)) * Propose changes to assertions setup with varying test client ([#3406](#3406)) ([fb27f6a](fb27f6a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
Changes
Fixes null comment case in the account resource described in #3402. Because of the limitation in the terraform testing framework, I couldn't create an account externally and then import the created account as a first step of the test (because the testing framework seems to be only working on already existing objects that can be found in-state). That's why I had to test this change manually by using
make install-tf
and using a locally built provider. I confirm that before accounts created with explicitly null commentCOMMENT = null
were failing and after applying this change they are not failing anymore.