fix: for 1624 resource monitor timestamps are always considered to have changed #2214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @RichBarnes, thanks for the PR! I will add review comments (and one of @sfc-gh-jcieslak @sfc-gh-swinkler will also take a look). |
sfc-gh-asawicki
requested review from
sfc-gh-swinkler,
sfc-gh-jcieslak and
sfc-gh-asawicki
sfc-gh-asawicki
requested changes
Nov 28, 2023
sfc-gh-jcieslak
requested changes
Dec 5, 2023
sfc-gh-jcieslak
previously approved these changes
Dec 11, 2023
sfc-gh-asawicki
previously approved these changes
Dec 12, 2023
sfc-gh-jcieslak
previously approved these changes
Dec 12, 2023
|
@RichBarnes you have 2 approves but still 2 conflicts that must be resolved before merging. :) |
Conflicts should be fixed, had issues with my local branch/git :) |
sfc-gh-asawicki
dismissed stale reviews from sfc-gh-jcieslak and themself
via
cbc8aeb
sfc-gh-asawicki
approved these changes
Dec 13, 2023
|
/ok-to-test sha=cbc8aeb3a8b63a3a3925234256203139a8ccf742 |
|
Integration tests failure for cbc8aeb3a8b63a3a3925234256203139a8ccf742 |
sfc-gh-jcieslak
approved these changes
Dec 13, 2023
|
@RichBarnes merged! Thanks for the contribution. 🎊 We will release it with the new version today or tomorrow. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Three changes:
validation logic requires both frequency and start_timestamp to be retrieved if either has been altered. If start_timestamp is set to immediately it has always changed on second terraform apply. Updated to set the haschanged boolean if either has been modified to correctly pass validation.
When start_timestamp is set to a specific date time, this is stored in Snowflake as a UTC date with a local offset. This causes an incorrect date to be returned and written to the tf state file. Added code to parse returned UTC back into a local format.
previous suspend_trigger naming support appears to cause issues once the start_timestamp problems are fixed. Changed logic to and rather then or's which resolved the issue.
Noticed a separate issue with the sql for trigger changes, if credit_quota hasn't been set then that line isn't included in the SQL builder which results in malformed sql and an error back from Snowflake. This should be raised as a separate fix/bug ticket.
The minor notify_users attribute behaviour should be resolved under another fix ticket.
Test Plan
Setting start_timestamp to IMMEDIATELY or a change to start_timestamp and running terraform apply twice reproduces the issue. Post fix, changing these values results in desired behaviour and no errors.
Ran existing tests as only logic minor changes to hasChanged logic and timestamp retrieval.
References
Allow closure of #1624