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

feat: Password policy resource #1702

Merged
merged 8 commits into from
Apr 12, 2023

Conversation

sfc-gh-ngaberel
Copy link
Contributor

Add support for the Password policy resource.
Fixes #1581

Test Plan

  • acceptance tests
  • password policy builder unit tests

@sfc-gh-ngaberel sfc-gh-ngaberel force-pushed the ngaberel-snow-770935-password-policy branch from 5f86cbf to a04f274 Compare April 11, 2023 00:07
pkg/snowflake/builder.go Outdated Show resolved Hide resolved
pkg/snowflake/password_policy.go Outdated Show resolved Hide resolved
pkg/snowflake/builder.go Outdated Show resolved Hide resolved
@sfc-gh-ngaberel sfc-gh-ngaberel changed the title Password policy resource feat: Password policy resource Apr 11, 2023
@github-actions
Copy link

Integration tests failure for 5f86cbf8e2b9c6325c750af6b11f90e3ff79e5b2

@github-actions
Copy link

Integration tests failure for a04f2741706a16397623e9b87aa9af5ba67605ea

@github-actions
Copy link

Integration tests success for ce5cb24749cf2a64acb2c0335073d95aacd3b696

pkg/resources/password_policy.go Outdated Show resolved Hide resolved
pkg/resources/password_policy.go Outdated Show resolved Hide resolved
pkg/resources/password_policy.go Outdated Show resolved Hide resolved
pkg/resources/password_policy.go Outdated Show resolved Hide resolved
pkg/resources/password_policy.go Outdated Show resolved Hide resolved
pkg/snowflake/builder.go Outdated Show resolved Hide resolved
return ok
}

func getFieldValue(props Props, fieldName string) (*reflect.Value, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

some of this reflection stuff is fairly complex. can we have tests for all these helper functions?

pkg/snowflake/password_policy.go Outdated Show resolved Hide resolved
pkg/snowflake/password_policy.go Outdated Show resolved Hide resolved
pkg/snowflake/password_policy.go Outdated Show resolved Hide resolved
}

type NewBuilder struct {
entityType string
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should really be an Enum, since there is a fixed quantity of entityTypes. Also entity isnt really the correct word, its actually "object" We have account level, database level, schema level objects.

pkg/snowflake/password_policy.go Outdated Show resolved Hide resolved

func (m *PasswordPolicyManager) Update(x *PasswordPolicyUpdateInput) (string, error) {
return m.genericBuilder.Alter(x)
}

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
File is not gofumpt-ed (gofumpt)

Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a way to automatically run gofumpt whenever a PR is made? this is a really annoying error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll add that with the vscode config changes.

@github-actions
Copy link

Integration tests failure for 6dbd13d41b226730010aec3ab55da4693bf7d4cc

@sfc-gh-ngaberel sfc-gh-ngaberel force-pushed the ngaberel-snow-770935-password-policy branch from 8112108 to 918ef60 Compare April 11, 2023 22:00
@sfc-gh-ngaberel sfc-gh-ngaberel force-pushed the ngaberel-snow-770935-password-policy branch from 918ef60 to 7edc7c7 Compare April 11, 2023 22:01
@github-actions
Copy link

Integration tests failure for 81121084487a082aecf6d6db412fcd62200428a4

@github-actions
Copy link

Integration tests success for 918ef6056cf8cf47b4d5ba168637a349245fb11e

@github-actions
Copy link

Integration tests success for 7edc7c7caf36e8d24a7cb85186d768fd822fa9bd

pkg/resources/password_policy.go Outdated Show resolved Hide resolved
pkg/resources/password_policy.go Outdated Show resolved Hide resolved

// CreatePasswordPolicy implements schema.CreateFunc.
func CreatePasswordPolicy(d *schema.ResourceData, meta interface{}) error {
manager, err := snowflake.NewPasswordPolicyManager()
Copy link
Collaborator

Choose a reason for hiding this comment

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

the word "manager" is not very descriptiive or useful. What i was thinking is that we could have a Client struct that has references to each object API. e.g.

type Client struct {
	conn *sql.DB

	Users           Users
	Roles           Roles
	Warehouses      Warehouses
	Databases       Databases
	Schemas         Schemas
	Tables          Tables
	NetworkPolicies NetworkPolicies
}

where Roles for example would be:

// Roles describes all the roles related methods that the
// Snowflake API supports.
type Roles interface {
	// List all the roles by pattern.
	List(ctx context.Context, options RoleListOptions) ([]*Role, error)
	// Create a new role with the given options.
	Create(ctx context.Context, options RoleCreateOptions) (*Role, error)
	// Read an role by its name.
	Read(ctx context.Context, role string) (*Role, error)
	// Update attributes of an existing role.
	Update(ctx context.Context, role string, options RoleUpdateOptions) (*Role, error)
	// Delete a role by its name.
	Delete(ctx context.Context, role string) error
	// Rename a role name.
	Rename(ctx context.Context, old string, new string) error
}

then you could reference password policeis as: client.PasswordPolicies.Create(<CreatePasswordPolicyInput>)

I also think addtion a validation method to the input would be useful. Example:

// RoleCreateOptions represents the options for creating a role.
type RoleCreateOptions struct {
	*RoleProperties

	// Required: Identifier for the role; must be unique for your account.
	Name string
}

func (o RoleCreateOptions) validate() error {
	if o.Name == "" {
		return errors.New("name must not be empty")
	}
	return nil
}

then you could validate input before doing generic request as so:

func (r *roles) List(ctx context.Context, options RoleListOptions) ([]*Role, error) {
	if err := options.validate(); err != nil {
		return nil, fmt.Errorf("validate list options: %w", err)
	}
...

}

db := meta.(*sql.DB)
_, err = db.Exec(stmt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

still not fond of having this db.Exec right here. Would be better to put this logic in the client struct (see earlier comment)

return fmt.Errorf("error executing create statement: %w", err)
}

d.SetId(PasswordPolicyID(&input.PasswordPolicy))
Copy link
Collaborator

Choose a reason for hiding this comment

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

a lot of resources share the same kind of ID format. This could probably be abstracted. Also i am not sure it needs to accept the whole PasswordPolicy reference, it really just needs the schema object identifier.

pkg/snowflake/builder.go Outdated Show resolved Hide resolved
pkg/snowflake/builder.go Show resolved Hide resolved
pkg/snowflake/builder.go Outdated Show resolved Hide resolved
pkg/snowflake/builder.go Show resolved Hide resolved

func (m *PasswordPolicyManager) Update(x *PasswordPolicyUpdateInput) (string, error) {
return m.genericBuilder.Alter(x)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a way to automatically run gofumpt whenever a PR is made? this is a really annoying error message

Co-authored-by: Scott Winkler <scott.winkler@snowflake.com>
@github-actions
Copy link

Integration tests success for 906a067f31363477bd3c22421a811676d2b813ac

@github-actions
Copy link

Integration tests success for f8ea30aa45ef179a82503452d2ff4e88fe18c30d

reflect.TypeOf(PasswordPolicyCreateInput{}),
reflect.TypeOf(PasswordPolicyUpdateInput{}),
reflect.TypeOf(PasswordPolicyUpdateInput{}),
reflect.TypeOf(PasswordPolicyDeleteInput{}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

i really don't like this way of registering functions with the password policy manager. i am sure there is a better way to do this.

@sfc-gh-swinkler sfc-gh-swinkler merged commit 7ee293b into main Apr 12, 2023
@sfc-gh-swinkler sfc-gh-swinkler deleted the ngaberel-snow-770935-password-policy branch April 12, 2023 17:37
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.

Password Policy Object
2 participants