Skip to content

Fix dynamic representation of zero values in maps and slices - #1154

Merged
shreyas-goenka merged 6 commits into
mainfrom
fix-scalar-zero
Jan 31, 2024
Merged

Fix dynamic representation of zero values in maps and slices#1154
shreyas-goenka merged 6 commits into
mainfrom
fix-scalar-zero

Conversation

@shreyas-goenka

@shreyas-goenkashreyas-goenka commented Jan 25, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Changes

In the dynamic configuration, the nil value (dyn.NilValue) denotes a value that should not be serialized, ie a value being nil is the same as it not existing in the first place.

This is not true for zero values in maps and slices. This PR fixes the conversion from typed values to dyn.Value, to treat zero values in maps and slices as zero and not nil.

Tests

Unit tests

@codecov-commenter

codecov-commenter commented Jan 25, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6fcf6ba) 51.29% compared to head (ff51fec) 51.49%.
Report is 5 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #1154 +/- ##
==========================================
+ Coverage 51.29% 51.49% +0.19% 
==========================================
Files 292 292 Lines 16322 16333 +11 ==========================================
+ Hits 8373 8411 +38 + Misses 7340 7315 -25 + Partials 609 607 -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This was referenced Jan 25, 2024
Closed
Merged
andrewnester
andrewnester reviewed Jan 30, 2024
View reviewed changes
Comment threadlibs/dyn/convert/end_to_end_test.go Outdated
Comment threadlibs/dyn/convert/from_typed.go
Comment threadlibs/dyn/convert/from_typed.go Outdated
pietern
pietern reviewed Jan 31, 2024
View reviewed changes
Comment threadlibs/dyn/convert/from_typed.go Outdated
Comment threadlibs/dyn/convert/from_typed.go
Comment threadlibs/dyn/convert/from_typed_test.go
func TestFromTypedStringStructWithZeroValue(t *testing.T) {
type Tmp struct {
Foo string `json:"foo"`
Bar string `json:"bar"`

@pieternpieternJan 31, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This raises an interesting point; what about omitempty?

Can deal with this later, as it is not handled now either.

@shreyas-goenkashreyas-goenkaJan 31, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah fair point. I'm taking a look right now whether we need to handle it as a followup

pietern
pietern reviewed Jan 31, 2024
View reviewed changes
Comment threadlibs/dyn/convert/from_typed.go Outdated
Comment threadlibs/dyn/convert/from_typed.go
@shreyas-goenka
shreyas-goenka added this pull request to the merge queueJan 31, 2024
Merged via the queue into main with commit 6beda44Jan 31, 2024
@shreyas-goenka
shreyas-goenka deleted the fix-scalar-zero branch January 31, 2024 14:33
shreyas-goenka added a commit that referenced this pull request Feb 1, 2024
@shreyas-goenka
Revert "Fix dynamic representation of zero values in maps and slices (#…
89f1d29
…1154)"
This reverts commit 6beda44.
andrewnester added a commit that referenced this pull request Feb 7, 2024
@andrewnester
Release v0.212.4
50c8752
Bundles:
* Allow specifying executable in artifact section and skip bash from WSL ([#1169](#1169)).
* Added warning when trying to deploy bundle with `--fail-if-running` and running resources ([#1163](#1163)).
* Group bundle run flags by job and pipeline types ([#1174](#1174)).
* Make sure grouped flags are added to the command flag set ([#1180](#1180)).
* Add short_name helper function to bundle init templates ([#1167](#1167)).
Internal:
* Fix dynamic representation of zero values in maps and slices ([#1154](#1154)).
* Refactor library to artifact matching to not use pointers ([#1172](#1172)).
* Harden `dyn.Value` equality check ([#1173](#1173)).
* Ensure every variable reference is passed to lookup function ([#1176](#1176)).
* Empty struct should yield empty map in `convert.FromTyped` ([#1177](#1177)).
* Zero destination struct in `convert.ToTyped` ([#1178](#1178)).
* Fix integration test with invalid configuration ([#1182](#1182)).
* Use `acc.WorkspaceTest` helper from bundle integration tests ([#1181](#1181)).
@andrewnesterandrewnester mentioned this pull request Feb 7, 2024
Merged
github-merge-queueBot pushed a commit that referenced this pull request Feb 7, 2024
@andrewnester
Release v0.212.4 (#1183)
f6cdc75
Bundles:
* Allow specifying executable in artifact section and skip bash from WSL
([#1169](#1169)).
* Added warning when trying to deploy bundle with `--fail-if-running`
and running resources
([#1163](#1163)).
* Group bundle run flags by job and pipeline types
([#1174](#1174)).
* Make sure grouped flags are added to the command flag set
([#1180](#1180)).
* Add short_name helper function to bundle init templates
([#1167](#1167)).
Internal:
* Fix dynamic representation of zero values in maps and slices
([#1154](#1154)).
* Refactor library to artifact matching to not use pointers
([#1172](#1172)).
* Harden `dyn.Value` equality check
([#1173](#1173)).
* Ensure every variable reference is passed to lookup function
([#1176](#1176)).
* Empty struct should yield empty map in `convert.FromTyped`
([#1177](#1177)).
* Zero destination struct in `convert.ToTyped`
([#1178](#1178)).
* Fix integration test with invalid configuration
([#1182](#1182)).
* Use `acc.WorkspaceTest` helper from bundle integration tests
([#1181](#1181)).
denik pushed a commit that referenced this pull request May 20, 2026
@shreyas-goenka
Fix dynamic representation of zero values in maps and slices (#1154)
165eecc
## Changes
In the dynamic configuration, the nil value (dyn.NilValue) denotes a
value that should not be serialized, ie a value being nil is the same as
it not existing in the first place.
This is not true for zero values in maps and slices. This PR fixes the
conversion from typed values to dyn.Value, to treat zero values in maps
and slices as zero and not nil.
## Tests
Unit tests
denik pushed a commit that referenced this pull request May 20, 2026
@andrewnester
Release v0.212.4 (#1183)
a3c4786
Bundles:
* Allow specifying executable in artifact section and skip bash from WSL
([#1169](#1169)).
* Added warning when trying to deploy bundle with `--fail-if-running`
and running resources
([#1163](#1163)).
* Group bundle run flags by job and pipeline types
([#1174](#1174)).
* Make sure grouped flags are added to the command flag set
([#1180](#1180)).
* Add short_name helper function to bundle init templates
([#1167](#1167)).
Internal:
* Fix dynamic representation of zero values in maps and slices
([#1154](#1154)).
* Refactor library to artifact matching to not use pointers
([#1172](#1172)).
* Harden `dyn.Value` equality check
([#1173](#1173)).
* Ensure every variable reference is passed to lookup function
([#1176](#1176)).
* Empty struct should yield empty map in `convert.FromTyped`
([#1177](#1177)).
* Zero destination struct in `convert.ToTyped`
([#1178](#1178)).
* Fix integration test with invalid configuration
([#1182](#1182)).
* Use `acc.WorkspaceTest` helper from bundle integration tests
([#1181](#1181)).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pieternpieternpietern approved these changes
@andrewnesterandrewnesterAwaiting requested review from andrewnester

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shreyas-goenka@codecov-commenter@pietern@andrewnester