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

fix: Fix warehouse read and resource monitor empty set #2319

Merged
merged 6 commits into from
Jan 8, 2024
Merged

fix: Fix warehouse read and resource monitor empty set #2319

merged 6 commits into from
Jan 8, 2024

Conversation

sfc-gh-asawicki
Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki commented Jan 4, 2024 •
edited
Loading

Fix warehouse read and resource monitor empty set:

  • Warehouse object parameters were not read, so external changes to them were not recognized by the resource.
  • The resource monitor was always setting Set for alter (even when no properties were altered), which led to syntax errors on SQL generation.
  • The syntax described in the docs is not right. Added test to check ALTER behavior for just NOTIFY_USERS altered.

Fix #2318
Fix #2316

@sfc-gh-asawicki sfc-gh-asawicki marked this pull request as ready for review January 4, 2024 17:30
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 4, 2024

Integration tests failure for f177a73591421b7a0ac52a3860945c9b920e5a36

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 4, 2024

Integration tests failure for d008cceaed7ddfd65df6740ac34a0a073e678f03

sfc-gh-swinkler
sfc-gh-swinkler approved these changes Jan 8, 2024
View reviewed changes
pkg/sdk/testint/resource_monitors_integration_test.go
@@ -256,11 +280,11 @@ func TestInt_ResourceMonitorAlter(t *testing.T) {
alterOptions := &sdk.AlterResourceMonitorOptions{
Set: &sdk.ResourceMonitorSet{
CreditQuota: &creditQuota,
NotifyUsers: &sdk.NotifyUsers{
Users: []sdk.NotifiedUser{{Name: "ARTUR_SAWICKI"}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it a good idea to have your name here? can we not have a test user?

Copy link
Collaborator Author

@sfc-gh-asawicki sfc-gh-asawicki Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have written a comment about it here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/pkg/resources/email_notification_integration_acceptance_test.go#L13. I have not yet created the issue for it, but for now there is nothing wrong with keeping it on my user for the time being (it is required to have an email in some other tests and our service user does not have an email). As you can see I did not add this user as a part of this change, I only moved it around because of the SDK implementation change.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2024

Integration tests failure for d008cceaed7ddfd65df6740ac34a0a073e678f03

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2024

Integration tests failure for d008cceaed7ddfd65df6740ac34a0a073e678f03

@sfc-gh-asawicki sfc-gh-asawicki merged commit 05f96c6 into main Jan 8, 2024
8 of 9 checks passed
@sfc-gh-asawicki sfc-gh-asawicki deleted the fix-a-few-bugs branch January 8, 2024 13:18
@snowflake-release-please snowflake-release-please bot mentioned this pull request Jan 8, 2024
Merged
@sfc-gh-asawicki sfc-gh-asawicki mentioned this pull request Jan 10, 2024
Closed
sfc-gh-jcieslak pushed a commit that referenced this pull request Jan 11, 2024
@snowflake-release-please
chore(main): release 0.83.0 (#2305)
ac5dd36 
🤖 I have created a release *beep* *boop*
---


##
[0.83.0](v0.82.0...v0.83.0)
(2024-01-11)


### 🎉 **What's new:**

* Add create streamlit privilege to the SDK
([#2303](#2303))
([be01d5f](be01d5f))
* grant privileges to database role resource
([#2306](#2306))
([0311cf8](0311cf8))


### 🐛 **Bug fixes:**

* Add secondary account and fix tests
([#2324](#2324))
([da6ca73](da6ca73))
* external tables issues
([#2334](#2334))
([ae41691](ae41691))
* Fix test because of the date
([#2312](#2312))
([9a9ea33](9a9ea33))
* Fix warehouse read and resource monitor empty set
([#2319](#2319))
([05f96c6](05f96c6)),
closes
[#2318](#2318)
[#2316](#2316)
* goreleaser for mfa token caching
([#2320](#2320))
([4fef709](4fef709))

---
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-jcieslak sfc-gh-jcieslak sfc-gh-jcieslak approved these changes

@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
3 participants
@sfc-gh-asawicki @sfc-gh-swinkler @sfc-gh-jcieslak