[00:18] @smoser, I was able to test on artful successfully as well. I also addressed all the CR comments: https://code.launchpad.net/~dojordan/cloud-init/+git/cloud-init/+merge/334341 [00:29] :q [00:29] (sorry) [19:25] @smoser, another day another iteration pushed. Can you please take a look when you get a chance: https://code.launchpad.net/~dojordan/cloud-init/+git/cloud-init/+merge/334341 [19:25] blackboxsw: http://paste.ubuntu.com/26412425/ [19:25] what do you think of that [19:26] i'm basically out of things ic an complain about in dojordan's branch :) [19:26] so that paste, would simplify some things. [19:26] interested in dojordan's feedback too [19:26] and i'd like for blackboxsw to take a look at the branch there to see if i'ive missed anyting [19:27] (i really don't like 'unknown-245', ... that really should be handled centrally *somewhere* , but thats not dojordan's fault that it is how it is) [19:37] I like it, only comment is there are some differences between ec2 and azure that make a generic context manager not perfect [19:38] i.e. calculating the bcast addr, and the stupid unknown-245 dhcp option... [19:47] well, we can just h andle that if they're there. [19:48] er... in that the general one can use them. [19:48] if the dhcp server responded with those options, then... you probably should use them :) [19:50] true, i guess calculating the bcast addr is pretty generic. want me to incorporate your patch then? [19:51] yeah. if you're ok with that. [19:51] i really appreciate your willingness to take feedback [19:52] sounds good, and sure, it looks much cleaner [20:30] hrm sorry wrong channel [20:30] reading [20:37] I like the cool factor of nested context managers, but I wonder if we really even need EphemeralIPv4Network at all smoser. The only consumer was ec2, and azure would now be looking to use the new EphemeralDHCPv4 as well. [20:38] couldn't we just repurpose EphemeralIPv4Network to do all this for us? [20:42] I don't really have a strong concern, though existing pastebin doesn't properly accept the fallback_nic param and the context manager return value should be named lease instead of info for clarity [20:43] blackboxsw: well i think that probably digitalocean *should* use EphemeralIPv4Network [20:43] I do like encapsulating that maybe_dhcp handling inside the context manager [20:44] good point. [20:45] and nevermind on the accepting fallback_nic param. I saw that now in __init__ my bad [20:46] blackboxsw: i didn't know if i should do the dhcp in the __init__ or in the __enter__ [20:46] i could come up with arguments for both. [20:46] yeah, I *think* init makes sense here [20:47] and I think on line 41 you meant + if not self.ephipv4: [20:55] (stupid question) what benefit do you get from doing work in init vs enter? [20:55] i guess if you instantiated a context manager w/out a with statement? [20:58] I'm onboard with the introduction of the EphemeralDHCP context manager. that logic for interacting with EphemerlIPv4Network is kinda klunky. it's best to obfuscate that anyway from DatasourceEc2. [21:00] I actually think pylint prefers no __enter__ additional params as it looks like it raises an unexpected special method signature. I just had my wires crossed and thought we had generally set those params up in __enter__ instead of __init__ [21:03] I can't seem to find examples supporting extra params in __enter__ [21:07] so with CtxMgr(a): would call __init__ with a? [21:08] correct a would be passed in as param 1 to init [21:08] so theoretically, it's not possible to pass additional parms to __enter__? [21:09] and I can't create a context manager class where I'm able to do that.... but I'm just toying around as we speak. [21:12] dojordan: init versus __enter__ [21:12] its really just a usage thing [21:12] ahh so __init__ is performed at the same time as __enter__ when we use with statement [21:12] you could then do: [21:12] we could init separately and call with later proving the args I think [21:12] x = EphemeralDHCPv4("eth0") [21:12] with x: [21:12] ... [21:12] with x: [21:12] ... [21:12] (bad indentation there) [21:13] if you did the dhcp in __init__ then you wouldnt *re* dhcp each 'with' (__enter__) [21:13] if you only use the class in one 'with' then they're equivalent. [21:14] but you can't pass parameters to __enter__ but you can to init? [21:14] blackboxsw: when you do: [21:14] i think that is correct :) [21:14] dojordan: ^ [21:15] with myclass(name=value) as fp: [21:15] basically does: [21:15] .init(name=value) [21:15] then fp = .__enter__() [21:15] at least thats how i understand it [21:16] cool :) [21:16] next question, is it possible to call __exit__() and __enter__ again? [21:17] i.e. when we raise an exception, we go to the next loop iteration and create a new instance of the context manager. is there a better way? [21:20] well i fyou have the class around you can. [21:20] but as i did an that paste there isnt a lot of value in keeping the class around for re-use, right? [21:20] where as if it reused the old lease there would be value, but thats not what you want [21:21] right, i see [21:21] makes sense