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

chore: Continue random ids rework #2819

Merged
merged 22 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
60e5aa6
Add identifier interface conversion to FAQ
sfc-gh-asawicki May 15, 2024
e11cd46
Change current role helper response to the identifier
sfc-gh-asawicki May 16, 2024
9e4f942
Change string to identifier for database existence checker
sfc-gh-asawicki May 16, 2024
ee0704b
Move checking revoking privileges from share to other check destroy f…
sfc-gh-asawicki May 16, 2024
244a03d
Introduce materialized view helper and use it
sfc-gh-asawicki May 16, 2024
1f99de8
Clean materialized view acceptance tests a bit
sfc-gh-asawicki May 16, 2024
b5eed4b
Introduce DatabaseId for DatabaseObjectIdentifier
sfc-gh-asawicki May 16, 2024
a7390c2
Use Introduced DatabaseId and SchemaId
sfc-gh-asawicki May 16, 2024
37758b5
Move helpers from view acceptance test
sfc-gh-asawicki May 17, 2024
ef8038c
Extract precreated storage integration ids
sfc-gh-asawicki May 17, 2024
45461fb
Simplify id creation for schema
sfc-gh-asawicki May 17, 2024
6249726
Simplify id creation for table
sfc-gh-asawicki May 17, 2024
3b03063
Use identifiers directly in unsafe execute tests
sfc-gh-asawicki May 17, 2024
46e46a0
Change return type for current user helper
sfc-gh-asawicki May 17, 2024
6ecbb32
Add TODO for using context client
sfc-gh-asawicki May 17, 2024
f037fb5
Remove new id from resources package
sfc-gh-asawicki May 17, 2024
f4612e4
Finish NewAccountObjectIdentifier topics
sfc-gh-asawicki May 17, 2024
daef1cd
Add TODOs for procedure hardcoded identifiers
sfc-gh-asawicki May 17, 2024
2b7c8fa
Run make pre-push
sfc-gh-asawicki May 17, 2024
811cd29
Fix three tests
sfc-gh-asawicki May 17, 2024
5f2c544
Fix tests
sfc-gh-asawicki May 19, 2024
c07fea1
Fix after review
sfc-gh-asawicki May 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions CREATING_ISSUES.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,20 @@ which is highly recommended for large-scale migrations.

**Solution:** Some fields may expect different types of identifiers, when in doubt check [our documentation](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs) for the field or the [official Snowflake documentation](https://docs.snowflake.com/) what type of identifier is needed.

### Incorrect identifier type (panic: interface conversion)
**Problem** When getting stack traces similar to:
```text
panic: interface conversion: sdk.ObjectIdentifier is sdk.AccountObjectIdentifier, not sdk.DatabaseObjectIdentifier
```

[GitHub issue reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2779)

**Solution:** Some fields may expect different types of identifiers, when in doubt check [our documentation](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs) for the field or the [official Snowflake documentation](https://docs.snowflake.com/) what type of identifier is needed. Quick reference:
- AccountObjectIdentifier - `<name>`
- DatabaseObjectIdentifier - `<database>.<name>`
- SchemaObjectIdentifier - `<database>.<schema>.<name>`
- NewTableColumnIdentifier - `<database>.<schema>.<table>.<name>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it a constructor name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, fixed


### Incorrect account identifier (snowflake_database.from_share)
**Problem:** From 0.87.0 version, we are quoting incoming external account identifier correctly, which may break configurations that specified account identifier as `<org_name>.<acc_name>` that worked previously by accident.

Expand Down
35 changes: 35 additions & 0 deletions pkg/acceptance/check_destroy.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,41 @@ func CheckDatabaseRolePrivilegesRevoked(t *testing.T) func(*terraform.State) err
}
}

// CheckSharePrivilegesRevoked is a custom checks that should be later incorporated into generic CheckDestroy
func CheckSharePrivilegesRevoked(t *testing.T) func(*terraform.State) error {
t.Helper()
client := Client(t)

return func(state *terraform.State) error {
for _, rs := range state.RootModule().Resources {
if rs.Type != "snowflake_grant_privileges_to_share" {
continue
}
ctx := context.Background()

id := sdk.NewExternalObjectIdentifierFromFullyQualifiedName(rs.Primary.Attributes["to_share"])
grants, err := client.Grants.Show(ctx, &sdk.ShowGrantOptions{
To: &sdk.ShowGrantsTo{
Share: &sdk.ShowGrantsToShare{
Name: sdk.NewAccountObjectIdentifier(id.Name()),
},
},
})
if err != nil {
return err
}
var grantedPrivileges []string
for _, grant := range grants {
grantedPrivileges = append(grantedPrivileges, grant.Privilege)
}
if len(grantedPrivileges) > 0 {
return fmt.Errorf("share (%s) is still granted with privileges: %v", id.FullyQualifiedName(), grantedPrivileges)
}
}
return nil
}
}

// CheckUserPasswordPolicyAttachmentDestroy is a custom checks that should be later incorporated into generic CheckDestroy
func CheckUserPasswordPolicyAttachmentDestroy(t *testing.T) func(*terraform.State) error {
t.Helper()
Expand Down
4 changes: 2 additions & 2 deletions pkg/acceptance/helpers/context_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (c *ContextClient) CurrentAccount(t *testing.T) string {
return currentAccount
}

func (c *ContextClient) CurrentRole(t *testing.T) string {
func (c *ContextClient) CurrentRole(t *testing.T) sdk.AccountObjectIdentifier {
t.Helper()
ctx := context.Background()

Expand All @@ -52,7 +52,7 @@ func (c *ContextClient) CurrentRegion(t *testing.T) string {
return currentRegion
}

func (c *ContextClient) CurrentUser(t *testing.T) string {
func (c *ContextClient) CurrentUser(t *testing.T) sdk.AccountObjectIdentifier {
t.Helper()
ctx := context.Background()

Expand Down
2 changes: 1 addition & 1 deletion pkg/acceptance/helpers/database_role_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (c *DatabaseRoleClient) CleanupDatabaseRoleFunc(t *testing.T, id sdk.Databa

return func() {
// to prevent error when db was removed before the role
_, err := c.context.client.Databases.ShowByID(ctx, sdk.NewAccountObjectIdentifier(id.DatabaseName()))
_, err := c.context.client.Databases.ShowByID(ctx, id.DatabaseId())
if errors.Is(err, sdk.ErrObjectNotExistOrAuthorized) {
return
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/acceptance/helpers/dynamic_table_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (c *DynamicTableClient) CreateDynamicTable(t *testing.T, tableId sdk.Schema

func (c *DynamicTableClient) CreateDynamicTableWithOptions(t *testing.T, schemaId sdk.DatabaseObjectIdentifier, name string, warehouseId sdk.AccountObjectIdentifier, tableId sdk.SchemaObjectIdentifier) (*sdk.DynamicTable, func()) {
t.Helper()
id := sdk.NewSchemaObjectIdentifier(schemaId.DatabaseName(), schemaId.Name(), name)
id := sdk.NewSchemaObjectIdentifierInSchema(schemaId, name)
targetLag := sdk.TargetLag{
MaximumDuration: sdk.String("2 minutes"),
}
Expand Down
16 changes: 12 additions & 4 deletions pkg/acceptance/helpers/ids_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ func (c *IdsGenerator) AccountIdentifierWithLocator() sdk.AccountIdentifier {
return sdk.NewAccountIdentifierFromAccountLocator(c.context.client.GetAccountLocator())
}

func (c *IdsGenerator) newSchemaObjectIdentifier(name string) sdk.SchemaObjectIdentifier {
return sdk.NewSchemaObjectIdentifier(c.context.database, c.context.schema, name)
func (c *IdsGenerator) NewSchemaObjectIdentifier(name string) sdk.SchemaObjectIdentifier {
return sdk.NewSchemaObjectIdentifierInSchema(c.SchemaId(), name)
}

func (c *IdsGenerator) RandomAccountObjectIdentifier() sdk.AccountObjectIdentifier {
Expand All @@ -49,8 +49,16 @@ func (c *IdsGenerator) RandomAccountObjectIdentifierContaining(part string) sdk.
return sdk.NewAccountObjectIdentifier(c.AlphaContaining(part))
}

func (c *IdsGenerator) RandomDatabaseObjectIdentifier() sdk.DatabaseObjectIdentifier {
return sdk.NewDatabaseObjectIdentifier(c.DatabaseId().Name(), c.Alpha())
}

func (c *IdsGenerator) RandomSchemaObjectIdentifier() sdk.SchemaObjectIdentifier {
return c.newSchemaObjectIdentifier(c.Alpha())
return c.RandomSchemaObjectIdentifierInSchema(c.SchemaId())
}

func (c *IdsGenerator) RandomSchemaObjectIdentifierInSchema(schemaId sdk.DatabaseObjectIdentifier) sdk.SchemaObjectIdentifier {
return sdk.NewSchemaObjectIdentifierInSchema(schemaId, c.Alpha())
}

func (c *IdsGenerator) Alpha() string {
Expand All @@ -66,7 +74,7 @@ func (c *IdsGenerator) AlphaContaining(part string) string {
}

func (c *IdsGenerator) AlphaWithPrefix(prefix string) string {
return c.withTestObjectSuffix(prefix + c.Alpha())
return prefix + c.Alpha()
sfc-gh-jcieslak marked this conversation as resolved.
Show resolved Hide resolved
}

func (c *IdsGenerator) withTestObjectSuffix(text string) string {
Expand Down
3 changes: 1 addition & 2 deletions pkg/acceptance/helpers/masking_policy_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ func (c *MaskingPolicyClient) CreateMaskingPolicyWithOptions(t *testing.T, schem
t.Helper()
ctx := context.Background()

name := random.String()
id := sdk.NewSchemaObjectIdentifier(schemaId.DatabaseName(), schemaId.Name(), name)
id := c.ids.RandomSchemaObjectIdentifierInSchema(schemaId)

err := c.client().Create(ctx, id, signature, returns, expression, options)
require.NoError(t, err)
Expand Down
53 changes: 53 additions & 0 deletions pkg/acceptance/helpers/materialized_view_client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package helpers

import (
"context"
"testing"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/stretchr/testify/require"
)

type MaterializedViewClient struct {
context *TestClientContext
ids *IdsGenerator
}

func NewMaterializedViewClient(context *TestClientContext, idsGenerator *IdsGenerator) *MaterializedViewClient {
return &MaterializedViewClient{
context: context,
ids: idsGenerator,
}
}

func (c *MaterializedViewClient) client() sdk.MaterializedViews {
return c.context.client.MaterializedViews
}

func (c *MaterializedViewClient) CreateMaterializedView(t *testing.T, query string, orReplace bool) (*sdk.MaterializedView, func()) {
t.Helper()
return c.CreateMaterializedViewWithName(t, c.ids.RandomSchemaObjectIdentifier(), query, orReplace)
}

func (c *MaterializedViewClient) CreateMaterializedViewWithName(t *testing.T, id sdk.SchemaObjectIdentifier, query string, orReplace bool) (*sdk.MaterializedView, func()) {
t.Helper()
ctx := context.Background()

err := c.client().Create(ctx, sdk.NewCreateMaterializedViewRequest(id, query).WithOrReplace(sdk.Bool(orReplace)))
require.NoError(t, err)

view, err := c.client().ShowByID(ctx, id)
require.NoError(t, err)

return view, c.DropMaterializedViewFunc(t, id)
}

func (c *MaterializedViewClient) DropMaterializedViewFunc(t *testing.T, id sdk.SchemaObjectIdentifier) func() {
t.Helper()
ctx := context.Background()

return func() {
err := c.client().Drop(ctx, sdk.NewDropMaterializedViewRequest(id).WithIfExists(sdk.Bool(true)))
require.NoError(t, err)
}
}
4 changes: 1 addition & 3 deletions pkg/acceptance/helpers/password_policy_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"testing"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -44,8 +43,7 @@ func (c *PasswordPolicyClient) CreatePasswordPolicyInSchemaWithOptions(t *testin
t.Helper()
ctx := context.Background()

name := random.AlphanumericN(12)
id := sdk.NewSchemaObjectIdentifier(schemaId.DatabaseName(), schemaId.Name(), name)
id := c.ids.RandomSchemaObjectIdentifierInSchema(schemaId)

err := c.client().Create(ctx, id, options)
require.NoError(t, err)
Expand Down
3 changes: 1 addition & 2 deletions pkg/acceptance/helpers/pipe_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"testing"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -34,7 +33,7 @@ func (c *PipeClient) CreatePipeInSchema(t *testing.T, schemaId sdk.DatabaseObjec
t.Helper()
ctx := context.Background()

id := sdk.NewSchemaObjectIdentifier(schemaId.DatabaseName(), schemaId.Name(), random.AlphaN(20))
id := c.ids.RandomSchemaObjectIdentifierInSchema(schemaId)

err := c.client().Create(ctx, id, copyStatement, &sdk.CreatePipeOptions{})
require.NoError(t, err)
Expand Down
62 changes: 52 additions & 10 deletions pkg/acceptance/helpers/role_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,18 @@ func (c *RoleClient) client() sdk.Roles {
return c.context.client.Roles
}

func (c *RoleClient) UseRole(t *testing.T, roleName string) func() {
func (c *RoleClient) UseRole(t *testing.T, roleId sdk.AccountObjectIdentifier) func() {
t.Helper()
ctx := context.Background()

currentRole, err := c.context.client.ContextFunctions.CurrentRole(ctx)
require.NoError(t, err)

err = c.context.client.Sessions.UseRole(ctx, sdk.NewAccountObjectIdentifier(roleName))
err = c.context.client.Sessions.UseRole(ctx, roleId)
require.NoError(t, err)

return func() {
err = c.context.client.Sessions.UseRole(ctx, sdk.NewAccountObjectIdentifier(currentRole))
err = c.context.client.Sessions.UseRole(ctx, currentRole)
require.NoError(t, err)
}
}
Expand All @@ -53,9 +53,14 @@ func (c *RoleClient) CreateRoleWithName(t *testing.T, name string) (*sdk.Role, f

func (c *RoleClient) CreateRoleGrantedToCurrentUser(t *testing.T) (*sdk.Role, func()) {
t.Helper()
ctx := context.Background()

role, roleCleanup := c.CreateRole(t)
c.GrantRoleToCurrentUser(t, role.ID())

currentUser, err := c.context.client.ContextFunctions.CurrentUser(ctx)
require.NoError(t, err)

c.GrantRoleToUser(t, role.ID(), currentUser)
return role, roleCleanup
}

Expand All @@ -80,15 +85,12 @@ func (c *RoleClient) DropRoleFunc(t *testing.T, id sdk.AccountObjectIdentifier)
}
}

func (c *RoleClient) GrantRoleToCurrentUser(t *testing.T, id sdk.AccountObjectIdentifier) {
func (c *RoleClient) GrantRoleToUser(t *testing.T, id sdk.AccountObjectIdentifier, userId sdk.AccountObjectIdentifier) {
t.Helper()
ctx := context.Background()

currentUser, err := c.context.client.ContextFunctions.CurrentUser(ctx)
require.NoError(t, err)

err = c.client().Grant(ctx, sdk.NewGrantRoleRequest(id, sdk.GrantRole{
User: sdk.Pointer(sdk.NewAccountObjectIdentifier(currentUser)),
err := c.client().Grant(ctx, sdk.NewGrantRoleRequest(id, sdk.GrantRole{
User: sdk.Pointer(userId),
}))
require.NoError(t, err)
}
Expand All @@ -114,6 +116,31 @@ func (c *RoleClient) GrantOwnershipOnAccountObject(t *testing.T, roleId sdk.Acco
require.NoError(t, err)
}

// TODO: move later to grants client
func (c *RoleClient) RevokeCurrentGrantsFromSchemaObject(t *testing.T, roleId sdk.AccountObjectIdentifier, objectId sdk.SchemaObjectIdentifier, objectType sdk.ObjectType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename it to MoveOwnershipAndRevokeCurrentGrantsFromSchemaObject? It's a long name, but it mentions that it's calling grant ownership rather than revoke privileges from.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the signature to func (c *RoleClient) GrantOwnershipOnSchemaObject(t *testing.T, roleId sdk.AccountObjectIdentifier, objectId sdk.SchemaObjectIdentifier, objectType sdk.ObjectType, outboundPrivileges sdk.OwnershipCurrentGrantsOutboundPrivileges) instead

t.Helper()
ctx := context.Background()

err := c.context.client.Grants.GrantOwnership(
ctx,
sdk.OwnershipGrantOn{
Object: &sdk.Object{
ObjectType: objectType,
Name: objectId,
},
},
sdk.OwnershipGrantTo{
AccountRoleName: sdk.Pointer(roleId),
},
&sdk.GrantOwnershipOptions{
CurrentGrants: &sdk.OwnershipCurrentGrants{
OutboundPrivileges: sdk.Revoke,
},
},
)
require.NoError(t, err)
}

// TODO: move later to grants client
func (c *RoleClient) GrantPrivilegeOnDatabaseToShare(t *testing.T, databaseId sdk.AccountObjectIdentifier, shareId sdk.AccountObjectIdentifier) {
t.Helper()
Expand All @@ -122,3 +149,18 @@ func (c *RoleClient) GrantPrivilegeOnDatabaseToShare(t *testing.T, databaseId sd
err := c.context.client.Grants.GrantPrivilegeToShare(ctx, []sdk.ObjectPrivilege{sdk.ObjectPrivilegeReferenceUsage}, &sdk.ShareGrantOn{Database: databaseId}, shareId)
require.NoError(t, err)
}

// TODO: move later to grants client
func (c *RoleClient) ShowGrantsTo(t *testing.T, roleId sdk.AccountObjectIdentifier) []sdk.Grant {
t.Helper()
ctx := context.Background()

grants, err := c.context.client.Grants.Show(ctx, &sdk.ShowGrantOptions{
To: &sdk.ShowGrantsTo{
Role: roleId,
},
})
require.NoError(t, err)

return grants
}
3 changes: 1 addition & 2 deletions pkg/acceptance/helpers/stage_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"path/filepath"
"testing"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -58,7 +57,7 @@ func (c *StageClient) CreateStage(t *testing.T) (*sdk.Stage, func()) {

func (c *StageClient) CreateStageInSchema(t *testing.T, schemaId sdk.DatabaseObjectIdentifier) (*sdk.Stage, func()) {
t.Helper()
id := sdk.NewSchemaObjectIdentifier(schemaId.DatabaseName(), schemaId.Name(), random.AlphaN(8))
id := c.ids.RandomSchemaObjectIdentifierInSchema(schemaId)
return c.CreateStageWithRequest(t, sdk.NewCreateInternalStageRequest(id))
}

Expand Down
10 changes: 9 additions & 1 deletion pkg/acceptance/helpers/table_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (c *TableClient) CreateTableInSchema(t *testing.T, schemaId sdk.DatabaseObj
func (c *TableClient) CreateTableWithColumns(t *testing.T, schemaId sdk.DatabaseObjectIdentifier, name string, columns []sdk.TableColumnRequest) (*sdk.Table, func()) {
t.Helper()

id := sdk.NewSchemaObjectIdentifier(schemaId.DatabaseName(), schemaId.Name(), name)
id := sdk.NewSchemaObjectIdentifierInSchema(schemaId, name)
ctx := context.Background()

dbCreateRequest := sdk.NewCreateTableRequest(id, columns)
Expand Down Expand Up @@ -75,6 +75,14 @@ func (c *TableClient) DropTableFunc(t *testing.T, id sdk.SchemaObjectIdentifier)
}
}

func (c *TableClient) SetDataRetentionTime(t *testing.T, id sdk.SchemaObjectIdentifier, days int) {
t.Helper()
ctx := context.Background()

err := c.client().Alter(ctx, sdk.NewAlterTableRequest(id).WithSet(sdk.NewTableSetRequest().WithDataRetentionTimeInDays(sdk.Int(days))))
require.NoError(t, err)
}

// GetTableColumnsFor is based on https://docs.snowflake.com/en/sql-reference/info-schema/columns.
// TODO: extract getting table columns as resource (like getting tag in system functions)
func (c *TableClient) GetTableColumnsFor(t *testing.T, tableId sdk.SchemaObjectIdentifier) []InformationSchemaColumns {
Expand Down
Loading
Loading