Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Zero fields overriden in config files to avoid undesired merging logic #81

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Apr 27, 2021

Signed-off-by: Haytham Abuelfutuh [email protected]

TL;DR

The default behavior for the mapstructure package used to merge new and default configs is to merge maps by setting all keys and to merge arrays by replacing the first n elements with the new n elements while leaving the rest intact.

The intuitive behavior, talking to users, is that when a field is set in a config, it completely replaces the default config.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

There is a ZeroFields setting to be set in the decoder used. It only zeros fields that are being replaced from the config file and not just all fields...

Tracking Issue

flyteorg/flyte#948

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #81 (5342b5a) into master (f0e2256) will decrease coverage by 0.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   72.10%   71.74%   -0.36%     
==========================================
  Files          57       57              
  Lines        1975     1975              
==========================================
- Hits         1424     1417       -7     
- Misses        412      418       +6     
- Partials      139      140       +1     
Flag Coverage Δ
unittests 71.74% <ø> (-0.36%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cache/auto_refresh.go 73.68% <0.00%> (-7.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0e2256...5342b5a. Read the comment docs.

Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

thank you!

@EngHabu EngHabu merged commit 7631993 into master Apr 27, 2021
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants