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

Fix adding empty values to maps #1498

Closed
wants to merge 1 commit into from
Closed

Fix adding empty values to maps #1498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion pkg/tfbridge/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,17 @@ func makePropertyDiff(ctx context.Context, name, path string, v resource.Propert
name += ".%"
}
}
if d := tfDiff.Attribute(name); d != nil && d.Old != d.New {

if d := tfDiff.Attribute(name); d != nil &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of going with this, I could consider also detecting changes to map counts. The diff has an entry for tags.% that's changing, indicating the number of tags is changing, but is being ignored by makeDetailedDiff. Speaking of which this needs test cases also for changing a non-empty entry to empty and vise versa, and for removing an empty entry, the whole CRUD.

// Additional change detection: normally a non-nil record indicates that
// something is indeed changing, however it appears that Pulumi Set support
// currently relies on the additional check d.Old != d.New to suppress
// spurios set element diffs. Removing this check makes
// TestCollectionsWithMultipleItems Set tests to fail. This is preserved for
// now, but mitigaged by checking for d.Old == "" to fix adding an empty
// string that was not present before. Missing and empty values for strings
// are both represented as "".
(d.Old == "" || d.Old != d.New) {
other, hasOtherDiff := diff[path]

var arrDiff *shim.ResourceAttrDiff
Expand Down
55 changes: 55 additions & 0 deletions pkg/tfbridge/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2060,3 +2060,58 @@ func TestListNestedAddMaxItemsOne(t *testing.T) {
},
pulumirpc.DiffResponse_DIFF_SOME)
}

func TestAddingZeroString(t *testing.T) {
t.Skip("TODO currently failing")
diffTest(t,
// tfs
map[string]*schema.Schema{
"s": {Type: schema.TypeString, Optional: true},
"outp": {Type: schema.TypeString, Computed: true},
},
// info
nil,
// inputs
map[string]interface{}{"s": ""},
// state
map[string]interface{}{},
// expected
map[string]DiffKind{"s": A},
pulumirpc.DiffResponse_DIFF_SOME,
)
}

func TestAddingZeroStringToMap(t *testing.T) {
diffTest(t,
// tfs
map[string]*schema.Schema{
"tags": {
Type: schema.TypeMap,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"outp": {Type: schema.TypeString, Computed: true},
},
// info
nil,
// inputs
map[string]interface{}{
"tags": map[string]interface{}{
"x": "s",
"y": "",
},
},
// state
map[string]interface{}{
"tags": map[string]interface{}{
"x": "s",
},
},
// expected
map[string]DiffKind{
"tags.y": A,
},
pulumirpc.DiffResponse_DIFF_SOME,
)
}