-
Notifications
You must be signed in to change notification settings - Fork 19
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
Ns1 zone diff show #36
base: master
Are you sure you want to change the base?
Conversation
Hi, thanks for this, I've got my eye on it, and will do some careful review ASAP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, this looks pretty good. A minor quibble I wouldn't mention unless you were already gonna be in there, and one request:
The ruamel.yaml
dependency seems common and fine, but could you add a requirements.txt
with it pinned to whatever version (implementer's choice).
…equested by my teammate
want[param] = value | ||
else: | ||
if want[param] != value: | ||
want[param] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately i think comparison to params needs to be done recursively, or another approach, as value
can be a complex object (dictionary). The create secondary zone integration is failing, as the secondary
param's value is an object, and its uninitialized values (eg primary_port) are None, and our API is complaining about it.
this pattern seems applicable to records/other stuff, so it would make sense to me to factor this out into a common function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a whack - haven't really tested the corners, but maybe it's a start http://ix.io/2O0T/diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i should have used isinstance
instead of type
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @rupa! I appreciate the due diligence, you are correct and that is something I didn't notice. Your code looks good, I'll go through the proposed changes and make an update to this MR.
Allow Ansible module to show diffs in zone updates.
RESULTS
UNIT TESTING
ANSIBLE PLAYBOOK TEST RUN