| rharper | smoser: shall I land https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/343122 ? | 00:12 |
|---|---|---|
| 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:51 |
| smoser | rharper: yeah. | 00:52 |
| 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:53 |
| 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:55 |
| 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:57 |
| 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 | 00:58 |
| 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:00 |
| 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:01 |
| 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:04 |
| 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:05 |
| rharper | smoser: ok, pushing bash completion | 01:06 |
| 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:10 |
| blackboxsw | +1 rharper we can create a ./tool to gen that static file from recursive --help invocations | 01:31 |
| blackboxsw | good pt | 01:31 |
| blackboxsw | could do it at build time. or just run it on a branch in progress prior to commit | 01:32 |
| === pmcnabb_ is now known as pmcnabb | ||
| smoser | blackboxsw: have a minute ? | 16:13 |
| blackboxsw | yes definitely | 16:22 |
| blackboxsw | smoser: heading to hangout | 16:22 |
| 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:11 |
| 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:12 |
| smoser | are we consistent with o urselves now ? | 17:14 |
| smoser | between ifconfig and ip | 17:14 |
| smoser | blackboxsw: my freebsd seems not working on server stack :-( | 17:18 |
| * blackboxsw is trying freebsd on ec2 | 17:18 | |
| 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:41 |
| 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:43 |
| 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:44 |
| rharper | or something safer; | 17:45 |
| smoser | why set a default ? | 17:48 |
| smoser | they require input | 17:48 |
| 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:52 |
| 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:54 |
| 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:55 |
| 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:56 |
| 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:57 |
| 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 | 17:58 |
| mgerdts | smoser: thanks for landing those | 18:07 |
| blackboxsw | rharper: ec2 w/ ipv6 address | 18:20 |
| blackboxsw | https://pastebin.ubuntu.com/p/96qtpRpdxV/ | 18:20 |
| rharper | thx | 18:21 |
| rharper | so, the link-local is new, as is the lo in the routing v6 table | 18:21 |
| 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:22 |
| blackboxsw | yeah it was previously just . | 18:23 |
| blackboxsw | just absent I mean | 18:23 |
| smoser | blackboxsw: so what are you doing with all that ? | 18:43 |
| smoser | are we looking to make output identical to previous ? | 18:43 |
| 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:45 |
| 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:46 |
| smoser | i dont think line 29 is bad. | 18:47 |
| blackboxsw | also the whole table line 40+ (ipv6 routes) is 'new' | 18:48 |
| smoser | and i'm fine with 27 and 30 too | 18:48 |
| smoser | i think they're generally improvements. | 18:49 |
| 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:50 |
| 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:52 |
| 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:53 |
| rharper | blackboxsw: yeah, I agree with smoser | 18:56 |
| smoser | blackboxsw: can i help you with anything ? | 19:11 |
| blackboxsw | all good. just pushed all changes | 19:13 |
| 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:14 |
| blackboxsw | done smoser rharper https://pastebin.ubuntu.com/p/dK8JBmdpNk/ | 19:18 |
| blackboxsw | looks good | 19:18 |
| blackboxsw | I think we can land it and I'll put up a merge proposal for bionic if that sounds good? | 19:19 |
| blackboxsw | https://jenkins.ubuntu.com/server/job/cloud-init-ci/1025/ in progress | 19:21 |
| * blackboxsw will be back in 30 mins | 19:33 | |
| smoser | https://www.diffchecker.com/9sLtnCgT | 19:41 |
| smoser | blackboxsw: i put a patch on there. https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/342428 | 20:04 |
| blackboxsw | waiting on ci | 20:35 |
| smoser | blackboxsw: marked approved | 20:56 |
| blackboxsw | smoser: sorry power loss. I'm back up, landing and publishing | 21:21 |
| blackboxsw | and cutting a bionic upload MP | 21:22 |
| blackboxsw | smoser: it' | 21:29 |
| blackboxsw | s up https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/343562 | 21:29 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!