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.

Sign up for GitHub

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

Jump to bottom

chore: Migrate Warehouse resource + datasource to new SDK #1792

Merged
merged 8 commits into from
May 12, 2023
Merged

chore: Migrate Warehouse resource + datasource to new SDK #1792

merged 8 commits into from
May 12, 2023

Conversation

sfc-gh-ngaberel
Copy link
Contributor

Some API changes:

  • Tags field on snowflake_warehouse resource has been removed. Use snowflake_tag_association instead.
  • snowflake_warehouse resource fields warehouse_size, scaling_policy and warehouse_type now accept a subset of the options they previously accepted.

Test Plan

  • previous acceptance tests are mostly unchanged
  • new SDK is covered by SQL unit tests + full integration tests

References

@sfc-gh-ngaberel sfc-gh-ngaberel mentioned this pull request May 11, 2023
Merged
1 task
@github-actions
Copy link

Integration tests failure for ade0909ca94ca4004750cacb40cababc43c46332

@sfc-gh-ngaberel sfc-gh-ngaberel changed the title Migrate Warehouse resource + datasource to new SDK chore: Migrate Warehouse resource + datasource to new SDK May 12, 2023
@github-actions
Copy link

Integration tests failure for 132848f3ae7fdaa83cd7dcb3f2ee2563d0322af5

@github-actions
Copy link

Integration tests failure for ec32513b95fe0111f8c7abc1769306de1283235d

@github-actions
Copy link

Integration tests failure for c34ec5faad1b0f8dbd56fe4cc5c6ecaef432f3ce

@github-actions
Copy link

Integration tests failure for c89fd1c7cfbb2556682ee3f31ebf16bf11356c6c

@github-actions
Copy link

Integration tests success for ea4d55eddf4f15347509f3cc2c50e15798ff3e6d

@github-actions
Copy link

Integration tests success for 8ef0964ed68fcb2278f6e13dd33f1d8e71c91bd2

sfc-gh-swinkler
sfc-gh-swinkler reviewed May 12, 2023
View reviewed changes
pkg/sdk/warehouse.go
StatementTimeoutInSeconds *uint `ddl:"parameter" db:"STATEMENT_TIMEOUT_IN_SECONDS"`
}

type WarehouseUnsetField string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be better to use an Unset struct rather than making an enum type for this

pkg/sdk/warehouse.go
return fmt.Errorf("exactly one of Suspend, Resume, AbortAllQueries, NewName, Set, Unset, SetTags and UnsetTags must be set: %w", err)
}

if opts.Suspend != nil && *opts.Suspend && opts.Resume != nil && *opts.Resume {
Copy link
Collaborator

Choose a reason for hiding this comment

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

checkEclsuviePointers is a cool function. we should do something similiar for checking that both are not true. maybe making a rules validation lib

pkg/sdk/warehouse.go
Pendings string
Failed string
Suspended string
Uuid string
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be UUID

pkg/sdk/type_helpers.go
@@ -19,6 +19,16 @@ func Int(i int) *int {
return &i
}

// Uint returns a pointer to the given uint.
func Uint(i uint) *uint {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure that i like this over having int type

@sfc-gh-swinkler sfc-gh-swinkler merged commit a14b994 into main May 12, 2023
@sfc-gh-swinkler sfc-gh-swinkler deleted the warehouse-sdk branch May 12, 2023 18:49
@snowflake-release-please snowflake-release-please bot mentioned this pull request May 12, 2023
Merged
@astanfel
Copy link
Contributor

@sfc-gh-ngaberel This is a breaking change for existing declaration's of a given warehouse that are in lower case.

I got this error when testing another change I was working on

│ Error: expected warehouse_size to be one of [XSMALL SMALL MEDIUM LARGE XLARGE XXLARGE XXXLARGE X4LARGE X5LARGE X6LARGE], got xsmall

@ian-r-rose ian-r-rose mentioned this pull request May 31, 2023
Closed
@AnatEzra81
Copy link
Contributor

@sfc-gh-ngaberel this introduced a breaking change for the max_cluster_count warehouse property.
this pr allowed values greater than 10 since this is a soft limit. we are using warehouses with more clusters than 10 and cannot reduce the value because it will affect production load

@sfc-gh-ngaberel
Copy link
Contributor Author

Hi @AnatEzra81, thanks for reporting this, I'm talking to other teams to figure out what the validation rules should actually be (if any). Could you create a separate issue so we can track this?

@joedenniss joedenniss mentioned this pull request Jan 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@sfc-gh-swinkler sfc-gh-swinkler sfc-gh-swinkler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sfc-gh-ngaberel @astanfel @AnatEzra81 @sfc-gh-swinkler