[00:12] <rharper> smoser: shall I land https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/343122  ?
[00:51] <blackboxsw> I'm for  it as mentioned, I think we can do a followup with something dynamic if needed.
[00:51] <blackboxsw> I'd like us to continue spending time on the CLI, so there will be ample opportunity if needed to touch this again and make it dynamic at that point.
[00:51] <blackboxsw> as it is for now it's fast and correct and 'done' so I'm all for that
[00:52] <smoser> rharper: yeah.
[00:53] <blackboxsw> we can borrow  some simple regex cli --help parsing that landscape-api CLI did for the next iteration, it won't be too bad to pull in.
[00:53] <smoser> i consider myself more competant than one should be with shell scripting
[00:53] <smoser> and compgen stuff just makes me cringe
[00:55] <blackboxsw> seen here is another option. but yeah uses compgen too https://bazaar.launchpad.net/~landscape/landscape/trunk/view/head:/canonical/landscape/api/client/etc/bash_completion.d/landscape-api
[00:55] <blackboxsw> and yeah, I'm all for something simple if we could get away with it
[00:57] <blackboxsw> smoser: sorry I'm late on the review comments of ifconfig->iproute2 stuff, still massaging unit tests for the switch to look before you leap (i.e. which('ip') elif which('ifconfig') )
[00:58] <blackboxsw> FTR, I agree with the approach to testing for the cmd before running it. instead of all the CalledProcessError handling
[00:58] <smoser> blackboxsw: yeah, i knew that that was going to be a pain for your tests
[00:58] <smoser> one way or another we shoudl decide "we are using 'ip'" and then use it
[01:00] <blackboxsw> yeah right, instead of separate tests in each separate function (ip vs netstat    or  ip vs ifconfig)
[01:00] <blackboxsw> yeah just a has_iproute2 would be easy, only one thing to mock then.
[01:01] <blackboxsw> I might just make that change now, as it's too late to get this fixed and back to you for review tonight anyway
[01:04] <blackboxsw> meh, n/m I retract that statement, as we also check which(netstat) and which(ifconfig) and have different error messages/failure paths to handle, so the consolidation doesn't buy much as we'd have to still handle ProcessExecutionErrors and log warnings if the 2nd command we run isn't present anyway
[01:04] <blackboxsw> ahh well
[01:05] <smoser> freebsd uses ifconfig i believe ?
[01:05] <smoser> other than that i would be fine to assume that if you have netstat then you have ifconfig
[01:05] <smoser> they';re' both from net-tools
[01:06] <rharper> smoser: ok, pushing bash completion
[01:10] <rharper> blackboxsw: Yeah, regex parsing is one way though I think the parsers are relegated to other files, so we can't just grep through cmd/main.py;  the recursive --help invokation may be more robust and we can do that at build-time so we have a static bash file.
[01:31] <blackboxsw> +1 rharper we can create a ./tool to gen that static file from recursive --help invocations
[01:31] <blackboxsw> good pt
[01:32] <blackboxsw> could do it at build time. or just run it on a branch in progress prior to commit
[16:13] <smoser> blackboxsw: have a minute ?
[16:22] <blackboxsw> yes definitely
[16:22] <blackboxsw> smoser: heading to hangout
[17:11] <blackboxsw> rharper: smoser dropped Metric column from route info output to retain original behavior. Only differences in behavior to what currently exists in bionic when relying on ip instead of ifconfig/netstat are that we do the following:
[17:11] <blackboxsw> 1. properly print full scope name for addrs as presented by ip
[17:11] <blackboxsw> 2. drop trailing ':' in the device column
[17:12] <blackboxsw> 3. print ipv6 addrs instead of '.'
[17:12] <blackboxsw> 4. print an ipv6 routes table
[17:12] <blackboxsw> smoser: rharper the diff on an ec2 instance: https://pastebin.ubuntu.com/p/KZWYPbCRvC/
[17:14] <smoser> are we consistent with o urselves now ?
[17:14] <smoser> between ifconfig and ip
[17:18] <smoser> blackboxsw: my freebsd seems not working on server stack :-(
[17:18]  * blackboxsw is trying freebsd on ec2
[17:41] <rharper> blackboxsw: can you run ec2 with ipv6 addr so we can see the diff?  I didn;t think we displayed link-local ip6 addrs; and we don't split out v4 and v6 for netdev, but we do for route ?
[17:43] <blackboxsw> rharper spinning one up now
[17:43] <rharper> blackboxsw: w.r.t the interface to the various parse output methods;  as smoser has told me, if you don't want someone using the modules as an api, then you need to use _ prefix
[17:44] <rharper> otherwise, those methods in netinfo *are* an api, and that means callers could pass non-strings to you
[17:44] <blackboxsw> +1, I should change that
[17:44] <rharper> if smoser doesn't mind, then I'm OK with not adding checks on the inputs, but it seems reasonable to me to set those as input=None, and then add if not input: input = ""
[17:45] <rharper> or something safer;
[17:48] <smoser> why set a default ?
[17:48] <smoser> they require input
[17:52] <rharper> one way of ensuring you don't blow up doing .split() on a non string for typical usage, None is more comment than passing in a dict;  or raise valueerror on the wrong type
[17:52] <smoser> rharper: well, we really aren't providing an api. we're just not really doing that.
[17:52] <rharper> smoser: then it I think blackboxsw will end up marking the methods with _
[17:52] <smoser> we should, and ideally would. but just really are not.
[17:54] <smoser> rharper: there is no reason to provide a default variable
[17:54] <smoser> changing
[17:54] <smoser>  def netdev_info_iproute(ipaddr_out):
[17:54] <smoser> to
[17:54] <smoser>  def netdev_info_iproute(ipaddr_out=None)
[17:54] <smoser> is contrary to useful
[17:54] <rharper> sure
[17:54] <smoser> raising a TypeError if ipaddr_out is not a string is fine.
[17:55] <blackboxsw> yeah +1 not defaulting as we don't want users calling netdev_info_iproute(). it needs something to parse
[17:55] <blackboxsw> TypeError I balked at because our code is the only caller and it stuffs in util.subp output which is a known quantity of a string type
[17:56] <smoser> right.
[17:56] <smoser> so that is why i didnt' feel we need it.
[17:56] <blackboxsw> so it's a case where I didn't feel users could likely induce a failure path by presenting an invalid type
[17:57] <smoser> at this point we're not *really* providing an api.  no one that is using cloud-init as a python library (at thsi point) is surprised by that :)
[17:57] <smoser> that isnt that we shouldnt. just that we're imature.
[17:57] <blackboxsw> if we were, say, processing user-provided cloud-config information which we've loaded into a dict, then I'd be more concerned about checking types of data etc.
[17:57] <smoser> yeah, i largely agree with that.
[17:57] <smoser> but i'm also OK to just shove a _ on it too.
[17:58] <blackboxsw> yeah I'll do that _ as I feel like for our own sanity it's nice to designate what we think should really have internal (to the module) callers
[17:58] <blackboxsw> it's a good function/method documentation policy
[18:07] <mgerdts> smoser: thanks for landing those
[18:20] <blackboxsw> rharper: ec2 w/ ipv6 address
[18:20] <blackboxsw> https://pastebin.ubuntu.com/p/96qtpRpdxV/
[18:21] <rharper> thx
[18:21] <rharper> so, the link-local is new, as is the lo in the routing v6 table
[18:22] <rharper> should we clean those up?  unreachable seems unhelpful, on the fence on link-local ipv6 values, we're not showing link-local v4 stuff
[18:22] <smoser> link local is new ?
[18:22] <rharper> printing link-local values in the net dev info table
[18:22] <rharper> yeah, look at the 17.2 vs 18.2
[18:23] <blackboxsw> yeah it was previously just .
[18:23] <blackboxsw> just absent I mean
[18:43] <smoser> blackboxsw: so what are you doing with all that ?
[18:43] <smoser> are we looking to make output identical to previous ?
[18:45] <blackboxsw> smoser: rharper I just wanted to see if you guys had concerns about that behavior and if you think we should avoid printing link local info like line 29 of https://pastebin.ubuntu.com/p/96qtpRpdxV/
[18:46] <blackboxsw> and if line 27 and 30 should really be like 8 and 10
[18:46] <blackboxsw> do we want to continue to not process/report certain data, even though I fixed what I felt was a bug in the ommission
[18:46] <rharper> I dunno; my use of that table is mostly restricted to seeing what networking as applied, typically the ipv4/v6 addresses, I don't think for clouds many users will ahve a second instance in the same network where they could reach it via link local
[18:47] <smoser> i dont think line 29 is bad.
[18:48] <blackboxsw> also the whole table line 40+ (ipv6 routes) is 'new'
[18:48] <smoser> and i'm fine with 27 and 30 too
[18:49] <smoser> i think they're generally improvements.
[18:50] <smoser> blackboxsw: for the ipv6 output
[18:50] <smoser> i think there is no reasno to show routes on interface lo
[18:50] <smoser> (they're not seen on ipv4)
[18:52] <blackboxsw> +1 will drop them
[18:52] <blackboxsw> for SRU fodder, would we want to drop the ipv6 route table altogether
[18:52] <blackboxsw> ?
[18:53] <blackboxsw> or it's just more output that users could take advantage of (not really change in existing functionality presented by other tables)
[18:53] <smoser> i think its fine. idont really believe any tools are likely scraping that.
[18:53] <blackboxsw> cool sound good rharper ?
[18:56] <rharper> blackboxsw: yeah, I agree with smoser
[19:11] <smoser> blackboxsw: can i help you with anything ?
[19:13] <blackboxsw> all good. just pushed all changes
[19:14] <blackboxsw> was going to make deb one more time and test lxc and ec2 while waiting for ci
[19:14] <blackboxsw> smoser: rharper all done
[19:14] <rharper> nice
[19:14] <blackboxsw> will pastebin one more pass
[19:14] <smoser> rharper: i have updates to https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/343246 and responses to your comments
[19:18] <blackboxsw> done smoser rharper https://pastebin.ubuntu.com/p/dK8JBmdpNk/
[19:18] <blackboxsw> looks good
[19:19] <blackboxsw> I think we can land it and I'll put up a merge proposal for bionic if that sounds good?
[19:21] <blackboxsw> https://jenkins.ubuntu.com/server/job/cloud-init-ci/1025/ in progress
[19:33]  * blackboxsw will be back in 30 mins
[19:41] <smoser> https://www.diffchecker.com/9sLtnCgT
[20:04] <smoser> blackboxsw: i put a patch on there. https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/342428
[20:35] <blackboxsw> waiting on ci
[20:56] <smoser> blackboxsw: marked approved
[21:21] <blackboxsw> smoser: sorry power loss. I'm back up, landing and publishing
[21:22] <blackboxsw> and cutting a bionic upload MP
[21:29] <blackboxsw> smoser: it'
[21:29] <blackboxsw> s up https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/343562