-
Notifications
You must be signed in to change notification settings - Fork 399
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: database grant read #2063
fix: database grant read #2063
Conversation
Integration tests success for 036ace51b99d49a14b78a7629922c93db71bd25e |
pkg/resources/grant_helpers.go
Outdated
@@ -187,6 +187,8 @@ func readGenericGrant( | |||
privileges.addString(grant.Privilege) | |||
// Reassign set back | |||
sharePrivileges[granteeNameStrippedAccount] = privileges | |||
default: | |||
log.Printf("[WARN] unexpected grantee type: %s", grant.GranteeType) |
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.
Let's add the DATABASE ROLE type as a separate branch in this switch statement, not the default one. We can leave the default one, too, for future grantee types, but I would prefer having it explicit.
pkg/resources/grant_helpers.go
Outdated
@@ -187,6 +187,8 @@ func readGenericGrant( | |||
privileges.addString(grant.Privilege) | |||
// Reassign set back | |||
sharePrivileges[granteeNameStrippedAccount] = privileges | |||
default: |
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 the default case fixes the issue ? Right now, the program behaviour is the same except it logs on unknown case in switch
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.
actually this was already fixed in https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/2037/files
released as part of 0.70.1.
I guess this PR is not needed. it does make it more explicit though
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.
Hmm, in the pr you've linked there's also only change with just logging which shouldn't break / fix anything, right ? I don't understand why this or linked pr is fixing anything
Integration tests failure for fbbbbe32f47686ec9bca0feeeb9b4d32055b493f |
Integration tests failure for 35a02b89f1697f9a0ff54a0cf7d1865e09579952 |
Integration tests failure for cb8a8ef959ed3489a7bc71a19df849486efc53ad |
Integration tests failure for 53adedbbc356ae34df46c726dc7983e816193044 |
Integration tests failure for b6cc73f7727de3f275c7e1b016d377e13bf61d4a |
fixes #1863
grantee type "database_role" was not recognized in the switch/case statement. adding a default fixes that.
before:
after: