| dojordan | @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:18 |
|---|---|---|
| dojordan | :q | 00:29 |
| dojordan | (sorry) | 00:29 |
| dojordan | @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 |
| smoser | blackboxsw: http://paste.ubuntu.com/26412425/ | 19:25 |
| smoser | what do you think of that | 19:25 |
| smoser | i'm basically out of things ic an complain about in dojordan's branch :) | 19:26 |
| smoser | so that paste, would simplify some things. | 19:26 |
| smoser | interested in dojordan's feedback too | 19:26 |
| smoser | and i'd like for blackboxsw to take a look at the branch there to see if i'ive missed anyting | 19:26 |
| smoser | (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:27 |
| dojordan | I like it, only comment is there are some differences between ec2 and azure that make a generic context manager not perfect | 19:37 |
| dojordan | i.e. calculating the bcast addr, and the stupid unknown-245 dhcp option... | 19:38 |
| smoser | well, we can just h andle that if they're there. | 19:47 |
| smoser | er... in that the general one can use them. | 19:48 |
| smoser | if the dhcp server responded with those options, then... you probably should use them :) | 19:48 |
| dojordan | true, i guess calculating the bcast addr is pretty generic. want me to incorporate your patch then? | 19:50 |
| smoser | yeah. if you're ok with that. | 19:51 |
| smoser | i really appreciate your willingness to take feedback | 19:51 |
| dojordan | sounds good, and sure, it looks much cleaner | 19:52 |
| blackboxsw | hrm sorry wrong channel | 20:30 |
| blackboxsw | reading | 20:30 |
| blackboxsw | 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:37 |
| blackboxsw | couldn't we just repurpose EphemeralIPv4Network to do all this for us? | 20:38 |
| blackboxsw | 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:42 |
| smoser | blackboxsw: well i think that probably digitalocean *should* use EphemeralIPv4Network | 20:43 |
| blackboxsw | I do like encapsulating that maybe_dhcp handling inside the context manager | 20:43 |
| blackboxsw | good point. | 20:44 |
| blackboxsw | and nevermind on the accepting fallback_nic param. I saw that now in __init__ my bad | 20:45 |
| smoser | blackboxsw: i didn't know if i should do the dhcp in the __init__ or in the __enter__ | 20:46 |
| smoser | i could come up with arguments for both. | 20:46 |
| blackboxsw | yeah, I *think* init makes sense here | 20:46 |
| blackboxsw | and I think on line 41 you meant + if not self.ephipv4: | 20:47 |
| dojordan | (stupid question) what benefit do you get from doing work in init vs enter? | 20:55 |
| dojordan | i guess if you instantiated a context manager w/out a with statement? | 20:55 |
| blackboxsw | 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. | 20:58 |
| blackboxsw | 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:00 |
| blackboxsw | I can't seem to find examples supporting extra params in __enter__ | 21:03 |
| dojordan | so with CtxMgr(a): would call __init__ with a? | 21:07 |
| blackboxsw | correct a would be passed in as param 1 to init | 21:08 |
| dojordan | so theoretically, it's not possible to pass additional parms to __enter__? | 21:08 |
| blackboxsw | 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:09 |
| smoser | dojordan: init versus __enter__ | 21:12 |
| smoser | its really just a usage thing | 21:12 |
| blackboxsw | ahh so __init__ is performed at the same time as __enter__ when we use with statement | 21:12 |
| smoser | you could then do: | 21:12 |
| blackboxsw | we could init separately and call with later proving the args I think | 21:12 |
| smoser | x = EphemeralDHCPv4("eth0") | 21:12 |
| smoser | with x: | 21:12 |
| smoser | ... | 21:12 |
| smoser | with x: | 21:12 |
| smoser | ... | 21:12 |
| smoser | (bad indentation there) | 21:12 |
| smoser | if you did the dhcp in __init__ then you wouldnt *re* dhcp each 'with' (__enter__) | 21:13 |
| smoser | if you only use the class in one 'with' then they're equivalent. | 21:13 |
| dojordan | but you can't pass parameters to __enter__ but you can to init? | 21:14 |
| smoser | blackboxsw: when you do: | 21:14 |
| smoser | i think that is correct :) | 21:14 |
| smoser | dojordan: ^ | 21:14 |
| smoser | with myclass(name=value) as fp: | 21:15 |
| smoser | basically does: | 21:15 |
| smoser | <myclass>.init(name=value) | 21:15 |
| smoser | then fp = <result>.__enter__() | 21:15 |
| smoser | at least thats how i understand it | 21:15 |
| dojordan | cool :) | 21:16 |
| dojordan | next question, is it possible to call __exit__() and __enter__ again? | 21:16 |
| dojordan | 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:17 |
| smoser | well i fyou have the class around you can. | 21:20 |
| smoser | but as i did an that paste there isnt a lot of value in keeping the class around for re-use, right? | 21:20 |
| smoser | where as if it reused the old lease there would be value, but thats not what you want | 21:20 |
| dojordan | right, i see | 21:21 |
| dojordan | makes sense | 21:21 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!