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

Refactor to use common codepath for table coalescing #5821

Merged
merged 3 commits into from Jun 12, 2019

Conversation

Projects
None yet
4 participants
@aeijdenberg
Copy link
Contributor

commented May 31, 2019

This PR is split out from #5185 and adds a 2nd commit to refactor the table coalescing to a single code path.

@helm-bot helm-bot added the size/L label May 31, 2019

@aeijdenberg aeijdenberg referenced this pull request May 31, 2019

Merged

Fix nested null value overrides #5185

1 of 1 task complete

@aeijdenberg aeijdenberg force-pushed the govau:refactortablecoalesce branch from c6b8d26 to 45750d6 Jun 4, 2019

@aeijdenberg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Rebased on master since #5185 is now merged.

Now the line counts more accurately reflect this as a net code removal, which was the intention.

@thomastaylor312
Copy link
Collaborator

left a comment

I really like how this simplifies things. Just some questions around the merge algorithm refactor to address. It would also be helpful to add unit tests to catch the edge cases where value types don't match.

Show resolved Hide resolved pkg/chartutil/values.go Outdated
Show resolved Hide resolved pkg/chartutil/values.go Outdated

I meant to still request the message change not approve

@scottrigby
Copy link
Member

left a comment

Just message change is needed to match behavior.

aeijdenberg added some commits Jan 18, 2019

Refactor to use common codepath for table coalescing
Stop mutating maps passed on input

Signed-off-by: Adam Eijdenberg <adam.eijdenberg@digital.gov.au>
Add test for existing merging behaviour of mixed types
Signed-off-by: Adam Eijdenberg <adam.eijdenberg@digital.gov.au>
Update warning to be more accurate
Signed-off-by: Adam Eijdenberg <adam.eijdenberg@digital.gov.au>

@aeijdenberg aeijdenberg force-pushed the govau:refactortablecoalesce branch from 45750d6 to 29a295e Jun 12, 2019

@helm-bot helm-bot added size/S size/L and removed size/L size/S labels Jun 12, 2019

@aeijdenberg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Have added the test above into a separate commit which I added first, so that we can see the tests pass before the refactor.

Then I re-based the refactor, and the warning message change commit on top.

I think that's all that's needed?

@thomastaylor312
Copy link
Collaborator

left a comment

Thanks for all the great work on this @aeijdenberg! We really appreciate it

@thomastaylor312 thomastaylor312 added this to the 2.15.0 milestone Jun 12, 2019

@thomastaylor312 thomastaylor312 merged commit 724eb3e into helm:master Jun 12, 2019

2 checks passed

DCO DCO
Details
ci/circleci: build Your tests passed on CircleCI!
Details

@aeijdenberg aeijdenberg deleted the govau:refactortablecoalesce branch Jun 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.