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

Bump mergo v0.3.8 #2277

Merged
merged 2 commits into from
Jan 24, 2020
Merged

Bump mergo v0.3.8 #2277

merged 2 commits into from
Jan 24, 2020

Conversation

zappy-shu
Copy link
Contributor

@zappy-shu zappy-shu commented Jan 21, 2020

- What I did

Bumps mergo from v0.3.7 to v0.3.8

full diff: imdario/[email protected]

includes:

- How I did it

Updated the vendor and added 2 special transforms to handle a change in how pointers in maps are handled in mergo: darccio/mergo@v0.3.7...v0.3.8#diff-a103b5fee5fb31abebf12ee067c01da8R153

These transforms ensure that:

  • ulimit config overrides correctly switch between single and soft/hard instead of merging the 2 together.
  • service network config aliases are replaced with the override list instead of the lists being appended.

- How to verify it

The existing unit tests cover the special transforms as they have been put in place to preserve the existing functionality

- Description for the changelog

Bumps mergo dependency from v0.3.7 to v0.3.8

- A picture of a cute animal (not mandatory but encouraged)

cat-2019-11-19

full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@silvin-lubecki
Copy link
Contributor

The linter is complaining 🦁

cli/compose/loader/merge.go:206:49: mergeUlimitsConfig - result 0 (error) is always nil (unparam)
func mergeUlimitsConfig(dst, src reflect.Value) error {
                                                ^
cli/compose/loader/merge.go:213:56: mergeServiceNetworkConfig - result 0 (error) is always nil (unparam)
func mergeServiceNetworkConfig(dst, src reflect.Value) error {

@zappy-shu zappy-shu force-pushed the bump_mergo_v0.3.8 branch 3 times, most recently from f8c1131 to 4adfad4 Compare January 21, 2020 16:12
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

@zappy-shu I know the bump breaks some tests in the cli and you added those 2 transform functions to fix it, but as the code is not that trivial to review and obviously to refactor too, do you think we could add specific tests cases on this part?

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@silvin-lubecki
Copy link
Contributor

@thaJeztah PTAL

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

left one nit about the imports, but otherwise LGTM

cli/compose/loader/merge_test.go Outdated Show resolved Hide resolved
Added transforms for when merging compose overrides to preserve the
functionality that was broken by bumping mergo to v1.3.8

This includes:
- Special transform for ulimits so single overrides both soft/hard and
the reverse
- Special transform for service network configs so the override replaces
all aliases

Signed-off-by: Nick Adcock <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

all green; merging!

@thaJeztah thaJeztah merged commit 7a0b138 into docker:master Jan 24, 2020
@thaJeztah thaJeztah added this to the next milestone Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants