-
Notifications
You must be signed in to change notification settings - Fork 39
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
Ensure consistent load order of domains and networks. #346
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
By processing JSON or YAML files via Ordered dicts., we ensure that domains are processed in the order they are specified in the relevant configuration files.
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,8 @@ Prefix: | |
eth1: {ip: !!python/unicode '192.168.202.2', network: !!python/unicode 'n1'} | ||
VNC port: null | ||
distro: !!python/unicode 'cirros' | ||
metadata: {} | ||
metadata: !!python/object/apply:collections.OrderedDict | ||
- [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I sincerely hope I actually fixed the test and not bypassed it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's ok, the array content (the second line) is the contents of the ordered dict, probably we might want to add also a test where there's actually content there to make sure that's actually ordered, though the name of the class there might be enough. |
||
root password: !!python/unicode '123456' | ||
snapshots: '' | ||
status: down | ||
|
@@ -23,7 +24,8 @@ Prefix: | |
eth1: {ip: !!python/unicode '192.168.202.3', network: !!python/unicode 'n1'} | ||
VNC port: null | ||
distro: !!python/unicode 'cirros' | ||
metadata: {} | ||
metadata: !!python/object/apply:collections.OrderedDict | ||
- [] | ||
root password: !!python/unicode 'cubswin:)' | ||
snapshots: '' | ||
status: down |
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.
Not an issue with this patch - but stdlib's copy.deepcopy will not work here?(replacing the entire 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.
@david-caro can give an explanation (which I remember was quite long) on why not. IIRC, it's not really doing only deepcopy?
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.
Let me investigate over the weekend, it's been a quite busy week :)
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 remember that there was some subtlety on why to use this function instead of copy.deepcopy, I vote for trying it out and replace this one if no issues arise, hopefully the code that depended on that subtlety is fixed/removed.
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.
@david-caro - this is the explanation you once gave me:
The copy.deepcopy function is not enough, as it's not doing what I intended
with the deepcopy one.
The issue is that, when writing yaml, you can do something like:
key1: val1
key2: val2
<<*: common_stuff
key1: customval1
<<*: common_stuff
key2: customval2
That will allow you to reuse some common definitions over and over on the same
yaml file, the problem is that it's actually a reference to the same objects,
so if you define a list as the common_stuff, then including it twice, will
create a pointer to the same list on both dictionaries, creating issues when
updating them specifically for each host. So the deepcopy function is meant to
remove those common pointers and create totally independent objects that don't
interlink so we can safely update each of them independently.
That does not happen with json, as, afaik, there's no supported way to share
common definitions like that.
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.
Hmm, that was a good explanation on why any deepcopy is needed, not on why use our own deepcopy :/, my bad.
An example of that issue:
I don't see any issue overall, I'll try replacing it with the stdlib one and do some tests, see if it breaks anything.
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.
Just replacing it with the stdlib copy.deepcopy does not seem to work, tests are failing for me after the change, will investigate: