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: add in more functionality for UpdateResourceMonitor #1456

Merged
merged 4 commits into from
Jan 10, 2023

Conversation

sfc-gh-jlove
Copy link
Contributor

@sfc-gh-jlove sfc-gh-jlove commented Jan 5, 2023

resource_monitor now can update without creation and delation for

  • notify_users
  • suspend_triggers
  • notify_triggers
  • set_for_account
  • warehouses

Also fixed a typo in a comment implents to implements

Test Plan

  • acceptance tests

References

@sfc-gh-jlove sfc-gh-jlove self-assigned this Jan 5, 2023
pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
pkg/snowflake/resource_monitor.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

Integration tests failure for f9529f4c8c6bf8876f3b0287f94ecd58dc90f2d8

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

Integration tests success for 284080a19c3c728f63f30cb4110b3c85398a38bc

@sfc-gh-jlove sfc-gh-jlove marked this pull request as ready for review January 5, 2023 01:43
pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
if runSetStatement {
if err := snowflake.Exec(db, stmt.Statement()); err != nil {
return fmt.Errorf("error updating resource monitor %v\n%w", id, err)
}
}

// Remove from account
if d.HasChange("set_for_account") && !d.Get("set_for_account").(bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be combined with the add to account. something like

if d.HasChange("set_for_account")
  old, new := d.GetChange("set_for_account")
 if new == nil {
  // remove from account
} else {
 // set on new account
}
f d.Get("set_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional the issue is you can't have it set for both an account and a warehouse. So if you are going from setting a resource monitor from the account level to the warehouse level or vice versa you have to make sure it is unset first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

in that case you should also use"ConflictsWith", it works like setting ForceNew on an attirbute. Will prevent people from setting both at the same time.

// ConflictsWith is a set of schema keys that conflict with this schema.
// This will only check that they're set in the _config_. This will not
// raise an error for a malfunctioning resource that sets a conflicting
// key.
ConflictsWith []string

pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
pkg/resources/resource_monitor.go Outdated Show resolved Hide resolved
pkg/snowflake/resource_monitor.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Integration tests success for eea37e6d8a40c945aaa570f461b4a137ccf32e4b

@sfc-gh-jlove
Copy link
Contributor Author

@sfc-gh-swinkler Addressed all your feedback.

if runSetStatement {
if err := snowflake.Exec(db, stmt.Statement()); err != nil {
return fmt.Errorf("error updating resource monitor %v\n%w", id, err)
}
}

// Remove from account
if d.HasChange("set_for_account") && !d.Get("set_for_account").(bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in that case you should also use"ConflictsWith", it works like setting ForceNew on an attirbute. Will prevent people from setting both at the same time.

// ConflictsWith is a set of schema keys that conflict with this schema.
// This will only check that they're set in the _config_. This will not
// raise an error for a malfunctioning resource that sets a conflicting
// key.
ConflictsWith []string

@github-actions
Copy link

Integration tests success for f63d1b5c94095f93a7624a5d30a157511061d3ce

@sfc-gh-swinkler sfc-gh-swinkler merged commit 2df570f into main Jan 10, 2023
@sfc-gh-swinkler sfc-gh-swinkler deleted the SNOW-710421 branch January 10, 2023 19:29
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.

3 participants