chore: Detect changes in lists and sets #3147

sfc-gh-jcieslak · 2024-10-22T09:42:04Z

Test cases for changes in lists and sets. From the given time, I only went through essential cases that consisted of:

Ignoring the order of list items after creation.
Having the ability to update an item while ignoring the order.
For the testing, I created a test resource because currently, we don't have anything to test more complex examples of certain resource behaviors (temporary providers we create for custom diff testing are not sufficient in this case). The resource is only added to the resource list whenever a special env is set (we could remove it entirely and leave some documentation in the resource file (and acc test file) on how to prepare for the tests). The imitation of external storage was done by creating a struct and its global instance (some of the things needed to be exported since external changes tested in acceptance tests needed to access those). Certain resource fields were researched to test different approaches, each is described in the implementation file.

Also added an assert on lists/sets that is able to assert items in order independent manner (it was needed for the tests of the proposals).

Note: Only lists were tested, because there was no major issue with them in current resources. For the lists the following issues were addressed: #420, #753

Next pr

Apply (parameterized tests in object renaming test cases) chore: Test support for object renaming #3130 (comment)

github-actions · 2024-10-22T09:44:58Z

Integration tests failure for 1c69f8c29b11f1950e78b9f5fb8490c46290e22a

github-actions · 2024-10-22T11:52:24Z

Integration tests failure for 6af7ade4a4a93a310b348e75aa80804e2f29e5f2

github-actions · 2024-10-22T13:08:19Z

Integration tests failure for 6d0b27341585bc868e9e147d0cd05673613971ca

github-actions · 2024-10-22T13:08:19Z

Integration tests failure for 906ab60c177061e631b95b65c772c2e9ddf761a2

github-actions · 2024-10-22T13:09:31Z

Integration tests failure for ef62598504f315266185bfa671a7dfc8bc9f40e0

pkg/acceptance/bettertestspoc/assert/commons.go

sfc-gh-jmichalak · 2024-10-24T06:59:55Z

pkg/acceptance/bettertestspoc/assert/commons.go

+						index, indexErr := strconv.Atoi(attrParts[0])
+						isIndex := indexErr == nil
+
+						if len(attrParts) > 1 && isIndex {


Doesn't it assume that we have a list of structs? Does it work for lists of simple types?

No, it should work for both nested and simple types

It would be great to include this in tests.

Not sure if that should be done here, I'll add a note in the assert generator read me to utilize this function and test it more thoroughly.

pkg/acceptance/bettertestspoc/assert/commons.go

pkg/resources/object_renaming_lists_and_sets.go

pkg/resources/object_renaming_lists_and_sets_acceptance_test.go

sfc-gh-jmichalak · 2024-10-24T09:17:07Z

pkg/resources/object_renaming_lists_and_sets.go

+				oldItemIsStillPresent := false
+
+				// Sizes of config and state may not be the same
+				if len(d.GetRawState().AsValueMap()[parentKey].AsValueSlice()) > index {


These functions are deeply nested - pls try using collections.Map, extracting some code and returning earlier.

Early returns are not possible in most cases (they would break the diff suppression). collections.Map is stylistic change, I would leave those at the end if there will be time to still make those adjustments to the proposal. Let's leave this as open for now.

sfc-gh-asawicki

Let's discuss today on the meeting

pkg/acceptance/bettertestspoc/assert/commons.go

pkg/resources/object_renaming_lists_and_sets.go

pkg/resources/object_renaming_lists_and_sets_acceptance_test.go

sfc-gh-jmichalak · 2024-10-31T11:59:41Z

pkg/resources/object_renaming_lists_and_sets_acceptance_test.go

+			{
+				Config: objectRenamingConfigManuallyOrderedList([]map[string]any{
+					{"name": "nameTwo", "type": "STRING", "order": 20},
+					{"name": "nameOne", "type": "TEXT", "order": 10},


Nit: it would be great to have named variables for these list elements. This could be done later.

I could use the structs used in the implementation and convert them underneath the config function. Next pr.

pkg/resources/object_renaming_lists_and_sets.go

sfc-gh-asawicki · 2024-11-04T13:17:43Z

pkg/resources/object_renaming_lists_and_sets.go

+			ObjectRenamingDatabaseInstance.List = slices.DeleteFunc(ObjectRenamingDatabaseInstance.List, func(item ObjectRenamingDatabaseListItem) bool {
+				shouldRemove := item == removedItem
+				if shouldRemove {
+					ObjectRenamingDatabaseInstance.ChangeLog.Removed = append(ObjectRenamingDatabaseInstance.ChangeLog.Removed, map[string]any{


nit (for readability - for sure not this PR): add a func returning such map from item + even more readable, also add a func to add new log entry and append it (ideally it should be a short logItemRemoval(item) or something similar)

Reasoning here would be, that the reader can focus on the essence of the implementation being tested

👍 I'll add this right after merging this one.

pkg/resources/object_renaming_lists_and_sets_acceptance_test.go

# Conflicts: # pkg/provider/provider.go

github-actions · 2024-11-05T11:14:06Z

Integration tests failure for b1df5b4a06eea6614b56bc873c4b2267d0c04391

## [0.98.0](v0.97.0...v0.98.0) (2024-11-08) Feature scope readiness for V1: [link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD) ([Roadmap reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#wrap-up-the-functional-scope)). :exclamation: Migration guide: [v0.97.0 -> v0.98.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0970--v0980) ### 🎉 What's new - New resources: - authentication_policy ([#3098](#3098)), references [#2880](#2880) - external_volume ([#3106](#3106)), partially references [#2980](#2980) - stream_on_directory_table ([#3129](#3129)) - stream_on_view ([#3150](#3150)) - primary_connection, secondary_connection ([#3162](#3162)) - secret_with_basic_authentication, secret_with_generic_string, secret_with_oauth_authorization_code_grant, secret_with_oauth_client_credentials ([#3110](#3110)), ([#3141](#3141)) - New data sources: - connections ([#3155](#3155)), ([#3173](#3173)) - secrets ([#3131](#3131)) - Reworked: - provider configuration hierarchy ([#3166](#3166)), references [#1881](#1881), [#2145](#2145), [#2925](#2925), [#2983](#2983), [#3104](#3104) - provider configuration fields ([#3152](#3152)) streams data source ([#3151](#3151)) - SDK upgrades: - Upgrade tag SDK ([#3126](#3126)) - Recreate streams when they are stale ([#3129](#3129)) ### 🔧 Misc - Add object renaming research summary ([#3172](#3172)) - Test support for object renaming ([#3130](#3130)), ([#3147](#3147)), ([#3154](#3154)) - Add tests to issue [#3117](#3117) ([#3133](#3133)) - New roadmap entry ([#3158](#3158)) - Test more authentication methods ([#3178](#3178)) - Minor fixes ([#3174](#3174)) ### 🐛 Bug fixes - Apply various fixes ([#3176](#3176)), this addresses BCR 2024_08, references [#2717](#2717), [#3005](#3005), [#3125](#3125), [#3127](#3127), [#3153](#3153) - Connection and secret data sources tests ([#3177](#3177)) - Fix grant import docs ([#3183](#3183)), resolves [#3179](https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/3179) - Fix user resource import ([#3181](#3181)) - Handle external type changes in stream resources ([#3164](#3164)) - Do not use OR REPLACE on initial creation in resources with copy_grants ([#3129](#3129)) - Address issue [#2201](#2201) by introducing new stream resources Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>

sfc-gh-jcieslak force-pushed the detect-changes-in-lists-and-sets branch 2 times, most recently from 6af7ade to 6d0b273 Compare October 22, 2024 11:03

sfc-gh-jcieslak added 11 commits October 22, 2024 13:24

wip

5b56e27

wip

8230302

wip

88fb0e0

Add deep hierarchy test cases

80542c7

wip

08939c2

wip

668bb16

wip

580f598

wip

ef40e65

wip

2d51ab2

wip

cc29f7c

wip

ef62598

sfc-gh-jcieslak force-pushed the detect-changes-in-lists-and-sets branch from 6d0b273 to ef62598 Compare October 22, 2024 11:24

sfc-gh-asawicki self-requested a review October 24, 2024 08:46

sfc-gh-jmichalak requested changes Oct 24, 2024

View reviewed changes

sfc-gh-asawicki requested changes Oct 24, 2024

View reviewed changes

sfc-gh-asawicki reviewed Oct 24, 2024

View reviewed changes

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

Changes after review

92732b0

sfc-gh-asawicki self-requested a review October 31, 2024 09:21

Changes to the first proposal after review

59f2921

sfc-gh-jmichalak reviewed Oct 31, 2024

View reviewed changes

sfc-gh-asawicki previously approved these changes Nov 4, 2024

View reviewed changes

sfc-gh-jcieslak added 2 commits November 5, 2024 10:04

Merge branch 'refs/heads/main' into detect-changes-in-lists-and-sets

fe3abbd

# Conflicts: # pkg/provider/provider.go

changes after review

b1df5b4

sfc-gh-jcieslak dismissed sfc-gh-asawicki’s stale review via b1df5b4 November 5, 2024 09:06

sfc-gh-jmichalak approved these changes Nov 5, 2024

View reviewed changes

sfc-gh-jcieslak merged commit c3edb79 into main Nov 5, 2024
8 of 9 checks passed

sfc-gh-jcieslak deleted the detect-changes-in-lists-and-sets branch November 5, 2024 10:49

snowflake-release-please bot mentioned this pull request Nov 5, 2024

chore(main): release 0.98.0 #3137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Detect changes in lists and sets #3147

chore: Detect changes in lists and sets #3147

sfc-gh-jcieslak commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

sfc-gh-jmichalak Oct 24, 2024

sfc-gh-jcieslak Oct 30, 2024

sfc-gh-jmichalak Oct 31, 2024

sfc-gh-jcieslak Nov 5, 2024

sfc-gh-jmichalak Oct 24, 2024

sfc-gh-jcieslak Oct 30, 2024

sfc-gh-asawicki left a comment

sfc-gh-jmichalak Oct 31, 2024

sfc-gh-jcieslak Nov 5, 2024

sfc-gh-asawicki Nov 4, 2024

sfc-gh-asawicki Nov 4, 2024

sfc-gh-jcieslak Nov 5, 2024

github-actions bot commented Nov 5, 2024

chore: Detect changes in lists and sets #3147

chore: Detect changes in lists and sets #3147

Conversation

sfc-gh-jcieslak commented Oct 22, 2024 • edited Loading

Next pr

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-asawicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 5, 2024

sfc-gh-jcieslak commented Oct 22, 2024 •

edited

Loading