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

Backport/2.8/57685 #57794

Merged
merged 3 commits into from Jun 24, 2019

Conversation

Projects
None yet
5 participants
@mmazur
Copy link
Contributor

commented Jun 13, 2019

SUMMARY

None of the unit tests for kubevirt actually did anything. Fix that.

Also fix the regression in merge_dicts() introduced by this change.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

kubevirt

mmazur added some commits Jun 13, 2019

kubevirt: enable/update tests + fix merge_dicts() (#57685)
* Actually run the unit tests and separate them into two files

* Re-add recursion to merge_dicts()

* Update tests to work with current code

(cherry picked from commit 51add5a)
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@pkliczewski

This comment has been minimized.

Copy link

commented Jun 13, 2019

+1

D[k] = _deepupdate(D[k], E[k])
else:
D[k] = E[k]
return D

This comment has been minimized.

Copy link
@abadger

abadger Jun 17, 2019

Member

I think you're overcomplicating and obfuscating this code. If you need recursive merging, add this loop to merge_dicts() rather than creating a new, nested function with obscured variable names.

Or, an even better option: talk with @sdoran and @sivel about moving ansible.utils.vars.merge_hash() somewhere in ansible.module_utils.common (perhaps ansible.module_utils.common.collections) and use that directly.

This comment has been minimized.

Copy link
@mmazur

mmazur Jun 18, 2019

Author Contributor

@abadger my immediate goal here is to remove the regression that 2.8.1 introduced. Being able to use more of ansible's code is a great idea, and I hope @samdoran and @sivel follow your suggestion, so I can use that on devel, but for now I need something that is quickly mergeable.

Ideally it would be this exact version, as otherwise I'll have to write three versions of this code:

  • the one currently on devel
  • the new one on devel once merge_hash() is available
  • and a completely new one, not used anywhere else, just for stable-2.8, just so I can fix a regression.

How long before 2.8.2 is released?

This comment has been minimized.

Copy link
@abadger

abadger Jun 18, 2019

Member

2.8.2 will be released Thursday, next week. Last day to merge backports for it is on Monday when I go home for the day.

Yeah, please fix this code as I mentioned above. You may be fixing the regression but you're doing it by re-introducing the obfuscation that was gotten rid of. You won't need to maintain three versions. Fix the code in devel so that the loop is a part of merge_dicts() as I specified and then add that commit to this backport. That will be the only one until merge_hash() is available.

This comment has been minimized.

Copy link
@mmazur

mmazur Jun 24, 2019

Author Contributor

Added a commit that drops the nested function, hope that's what you had in mind.

This comment has been minimized.

Copy link
@samdoran

samdoran Jun 24, 2019

Member

I was looking into moving ansible.utils.vars.merge_hash() and discovered we have ansible.module_utils.common.dict_transformations.dict_merge() that was added in #41471. I wonder if that function could be used in this case.

This comment has been minimized.

Copy link
@mmazur

mmazur Jun 24, 2019

Author Contributor

@samdoran yup, it's a drop–in replacement for the nested function Toshio wanted dropped.

@abadger modified my commit, now the nested function got replaced with the ansible–provided one. Unit tests pass, so it should work fine.

This comment has been minimized.

Copy link
@abadger

abadger Jun 24, 2019

Member

@mmazur, cool, do you have a pull request for devel as well? I can merge both today.

This comment has been minimized.

Copy link
@mmazur

@mmazur mmazur force-pushed the mmazur:backport/2.8/57685 branch from e76aec8 to d606a63 Jun 24, 2019

@ansibot ansibot added core_review and removed shipit labels Jun 24, 2019

@mmazur mmazur force-pushed the mmazur:backport/2.8/57685 branch 2 times, most recently from 989e63a to f8669bf Jun 24, 2019

@mmazur mmazur force-pushed the mmazur:backport/2.8/57685 branch from f8669bf to fc9a2e7 Jun 24, 2019

@abadger abadger merged commit d915594 into ansible:stable-2.8 Jun 24, 2019

1 check passed

Shippable Run 129215 status is SUCCESS.
Details
@abadger

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Merged for the 2.8.2 release. Thanks for putting up with my review requirements :-)

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.