[00:17] <Odd_Bloke> Hah.
[10:15] <meena> so, i didn't realize we have no fallback when blkid doesn't work in ds-identify, so here's a bug: https://bugs.launchpad.net/cloud-init/+bug/1901174
[10:15] <ubot5> Ubuntu bug 1901174 in cloud-init "ds-identify cannot identify disk-based resources on FreeBSD" [Undecided,New]
=== cpaelzer__ is now known as cpaelzer
=== dionysus70 is now known as dionysus69
[14:32] <minimal> Can anyone help with a issue related to testcases? I'm working on a PR and have written a testcase for it but getting the weird situation where the RHEL sysconfig output appears to be JSON rather than plain text and so doesn't match the expected output
[14:49] <meena> it's not json, it's the object
[14:58] <meena> minimal, it means you're comparing a serialised version of thing with an unserialised one
[15:13] <minimal> meena: I've just using the pre-existing functions in test_net.py that compare expected output with actual output
[15:28] <minimal> I think I'm just getting confused by the failure output and its actually worked as intended
[15:32] <minimal> I'm still a bit confused about the testing layout in test_net.py - I expected to be able to define an "input" network config v2 YAML and the expected ENI, Netplan, and sysconfig for it in one place and have all outputs tested in on go but now I realise there are separate classes for testing each of these output formats
[15:32] <minimal> so seems I need to write separate testcases for each of the output types
[15:38] <rharper> minimal: yes, for a new scenario in test_net.py,  you have an input, which is ingested into internal network_state; and then for each distro/renderer, there's an expected output;  so it sounds like you're on the right track;   alternatively you might chat with Odd_Bloke to see if there are suggestions on switching to a pytest parametrized style which could significantly improve how those tests get written;
[15:42] <minimal> rharper: what's confused me is that in test_net.py there's "dhcpv6_stateless" which contains expected_eni, expected_netplan, expected_sysconfig_opensuse, and expected_sysconfig_rhel which I copied/pasted (as well as the function test_dhcpv6_stateless_config that uses it) as a basic for my new testcase but when I ran tox only expected_sysconfig_rhel appears to be tested - none of the other outputs are checks so why are they defined/present.
[15:43] <rharper> minimal: a diff would help; but test inputs/outputs data are reused in differnt test Classes in that file
[15:43] <rharper> test_net.py is a *beast* and it's easy to not see where all of the call sites are due to the repeated pattern of the classes
[15:44] <minimal> yeah, I'm finding it hard to figure out how it works when its 5000+ lines long
[15:47] <rharper> minimal: exactly =?
[15:47] <rharper> =/
[15:48] <minimal> seems I need to put the function for ENI in class TestEniRoundTrip, not in class TestEniNetRendering as I was doing.....doh!
[15:48] <rharper> a small refactor could take the defined configs and put them in tests/data/test_net_configs.py;  and then each of the classes in test_net, could be test_net_classname.py and import what they needed from data/test_net_configs.py
[15:50] <minimal> certainly seems like refactoring is badly needed. I'm trying to do the right thing and create a testcase for functionality that's been broken for how knows how long as its not currently tested.
[15:50] <minimal> time to reach for some headache tablets........... ;-)
[15:51] <rharper> heh
[16:16] <Odd_Bloke> Yeah, I think those tests have been structured well in that the test data and testing code _are_ separate, but that approach still leads to a lot of boilerplate in "vanilla" Python unit testing (which I agree parameterisation could address).  Even "just" a refactor to split them into separate files as suggested would help a great deal, I think.
[16:24] <minimal> have figured it out now for eni and netplan so now need to make changes to sysconfig.py before I can test that
[16:26] <minimal> Odd_Bloke: ideally it would be nice to have a generic test function that then behind the scenes calls one-or-more or all of the specific output functions
[16:27] <minimal> so for "dhcp" for example the test_dhcp function would call test_netplan_dhcp, test_eni_dhcp etc but the test_dhcp would have an arg to let you select some of the outputs if you did'nt want them all called
[16:47] <Odd_Bloke> minimal: Are you familiar with pytest parameterisation (https://docs.pytest.org/en/stable/parametrize.html)?  It allows you to programmatically generate multiple instances of the same test with different inputs: I think this would map onto the current dicts we already have pretty well.
[16:52] <Odd_Bloke> So instead of (e.g.) TestNetplanRoundTrip having to have a bunch of copy/pasted definitions, we would instead define a single test method which is parameterised with `yaml` and `expected_netplan` from each of the dicts.  (Something like `@pytest.mark.parametrize("yaml,expected_netplan", [(entry["yaml"], entry["expected_netplan"]) for entry in NETWORK_CONFIGS.values()])`.)
[16:53] <Odd_Bloke> So anything new added to NETWORK_CONFIGS will automatically be picked up by those tests (and we lose a lot of boilerplate code too).
=== philroche_ is now known as philroche
[17:00] <minimal> Odd_Bloke: nor familiar with that but it sounds good. For each test you'd need to be able to define which of multiple inputs to each test (netconfig v1, v2, eni?) and which (or all) of the outputs
[17:08] <Odd_Bloke> minimal: I think you'd define a separate test for each of the dict keys: different renderers have different setup requirements for testing.
[17:10] <Odd_Bloke> And then (and now, for that matter) you can run only the Netplan tests with: pytest -k TestNetplanRoundTrip tests/unittests/test_net.py
[17:11] <minimal> Odd_Bloke: BTW what's your view on Mina's comments on PR 623?
[17:12] <Odd_Bloke> (Due to our pytest support matrix (our tests need to run on the version of pytest in Ubuntu 16.04), I don't think we can mark up each of the cases to be able to select quite as specifically as "the 'small' configuration on netplan".  Given that _all_ the test_net tests currently run in ~2s, I don't think that's too painful.)
[17:17] <smoser> warning, stupid question
[17:17] <smoser> meena: do you use systemd on freebsd ?
[17:21] <smoser> someone have time to review https://github.com/canonical/cloud-init/pull/622
[17:29] <meena> smoser, nope, we can't we don't have cgroups on freebsd
[17:29] <meena> https://www.freebsdnews.com/2018/08/21/benno-rice-the-tragedy-of-systemd-bsdcan-2018/ good talk on why freebsd could really use something like systemd
[17:30] <meena> we also don't have the probably easier to port Solaris contracts, which are roughly the same concept
[17:37] <Odd_Bloke> minimal: Have replied on #623. :)
[17:39] <minimal> Odd_Bloke: Thanks, will be glad to put that PR to bed :-)
[17:42] <smoser> meena: so why do you care about ds-identify ?
[17:42] <meena> smoser: because it's what guesses which cloud you're on, and on FreeBSD, it's ca 95% wrong.
[18:15] <meena> how the heck do i get the first "word" in a $line in bash? $line[0] does not cut it
[18:19] <meena> is bash the worst programming language?
[18:26] <meena> oh, never mind, this isn't bash, this is sh.
[18:28] <minimal> cut -f1 ?
[18:30] <meena> minimal: that'd be 3 cut calls
[18:30] <minimal> efficiency isn't shell's selling point :-)
[18:31] <meena> i thought efficiency was the point of ds-identify
[18:31] <meena> nothing else uses cut in that code
[18:31] <meena> i'm already using awk to normalise an output
[18:32] <meena> nothing else does that either!
[18:32] <meena> there's one call to grep
[18:32] <meena> no sed
[18:38] <meena> smoser: ping
[18:38] <smoser> meena: 2 minutes ?
[18:39] <smoser> (i have to go, in < 2 minutes . return in 40)
[18:41] <smoser> sorry. out.
[18:43] <meena> s'okay
[19:19] <smoser> meena: back
[19:24] <meena> smoser: https://github.com/canonical/cloud-init/pull/625
[19:25] <meena> i'm surprised you can do so much with ds-identify without shelling out…
[19:25] <meena> what i'm not surprised about is how the result looks (ugly, af)
[19:25] <smoser> i've written way to much shell in my life.
[19:32] <meena> smoser: examples of what the lines look like are in https://bugs.launchpad.net/cloud-init/+bug/1901174
[19:32] <ubot5> Ubuntu bug 1901174 in cloud-init "ds-identify cannot identify disk-based resources on FreeBSD" [Undecided,New]
[19:37] <meena> smoser: i don't understand this: "also... maybe we consider trying to render different versions ? freebsd and linux ?"
[19:51] <smoser> we could have a ds-identify on linux that only contained linux
[19:51] <smoser> and one for freebsd that only contained freebsd
[19:52] <smoser> was what i was suggesting.
[19:52] <smoser> and "render" one file ds-identify based on which platform was being installed for
[19:55] <meena> smoser: i'll leave this to more advanced developers of cloud-init
[19:56] <smoser> what you're doing is probably ok.
[19:56] <meena> yeah, i don't get blkid not found errors any more ;)
[19:56] <smoser> but i dont want more confusing code to read (than it already is) or slower runtimee
[19:57] <meena> i've now resolved two of your review complaints, https://github.com/canonical/cloud-init/pull/625
[20:10] <meena> so, i have 3 pull requests in the race right now, and all of them are going to break me because of tests.