| Odd_Bloke | smoser: https://github.com/canonical/cloud-init/pull/536 <-- the Oracle comment update I mentioned previously | 14:42 |
|---|---|---|
| minimal | Hi folks. Anyone able to give some advice about test_handler dealing with testcases where a cloudinit module writes to a specific file (rather than passed as a parameter) and where the testcase also needs to load the file afterwards to check expected contents? My original method of letting the test_handler create the directory required ("/etc/apk/") for the tests failed with a permission error. | 16:44 |
| rharper | minimal: there are a few unittests whic huse the FilesystemMocking class | 17:07 |
| rharper | minimal: I think the ntp handler test-case that writes out template files .. | 17:08 |
| rharper | minimal: also the reRoot() should let you write files and read them back in via the temp path | 17:08 |
| minimal | rharper: Hi. Not looking to mock the write but rather to override the filename used for write by the module and also used for loadfile by the test handler. | 17:30 |
| minimal | rharper: I had a look at many of the test_handlers but still haven't figured things out yet. | 17:31 |
| minimal | Tried using mkstemp in the setUp function of the class to create a file and then in the testcase function tried using "cc_apk_configure._write_repositories_file.repo_file = self.tmp" to override the filename defined by the repo_file string in the module | 17:34 |
| rharper | minimal: right, so reRoot() from one of the unittest classes, I'll look up; will change root into a tmp path, so you won't need to change your repo file, and it'll write to tmp_path as if it were root | 17:39 |
| rharper | minimal: so, if you use FilesystemMockingTestCase, and then use reRoot(); that should be enough; minimal also, I think if you look at tests/unittests/test_handler/test_handler_ntp.py:TestNtp(); I think that should do what you want | 17:42 |
| minimal | rharper: Thanks I'll take a look at that. Unrelated - who's the best person to talk to about the NTP module? I've added Busybox NTP support but several of the NTP testcases are failing as they assume full NTP functionality rather than Busybox's limited support. | 17:50 |
| rharper | minimal: I'm familiar with them | 17:59 |
| rharper | minimal: is this related to the pools vs servers bit you mentioned in the PR ? | 18:00 |
| minimal | The only thing in a ntp.conf file that Busybox's ntp can handle is a "server" line. Also I believe it can't handle "iburst" as part of a server line, so that various testcases that expect "pool" lines and "server" lines with "iburst" will not work for it. | 18:01 |
| minimal | I'd added Busybox NTP support as its lightweight and Alpine's a lightweight distro. Wondering whether I should have just stuck with only Chrony | 18:02 |
| rharper | minimal: did you provide a template for it? I forget | 18:04 |
| minimal | yes, again with only server lines | 18:04 |
| rharper | I see, so when the alpine template is used, the test-cases themselves expect more things present | 18:05 |
| rharper | I wonder if the templates need more logic | 18:05 |
| minimal | yes - the NTP testcases loop through through each of the supported software (i.e. ntp, chrony, etc) and for each then test each distro that supports that software | 18:06 |
| minimal | so they expect a generated ntp.conf to loop basically the same for Debian, Ubuntu, etc, and Alpine - which is the problem | 18:06 |
| minimal | oops, loop == look | 18:07 |
| rharper | yeah | 18:07 |
| minimal | I could have used Openntp in Alpine for the ntp support but its "old code", from memory the last upstream release was 12-18 months ago, plus Openntp is of similar size to Chrony, whereas for a lightweight distro needing just a NTP client daemon the Busybox NTP seemed like a good candidate | 18:08 |
| rharper | so, I think we need two things; 1) when on alpine rendering the template, if the user-data includes unsupported keys (pools e.g.) then cloud-init should emit warnings in the logs; 2) the unittests can be adjusted to handle alpine separately ; for example we could filter out alpine from the distro list in the module and then add new unittests just for alpine, providing supported user-data (servers) and unsupported | 18:10 |
| rharper | (servers/pools) and see that we log the warning | 18:10 |
| minimal | Yeah that's why when I realised the implications of Busybox ntp support I wondered who's be the person(s) to consider if it was worth it - I could always drop the (Busybox) "ntp" and just go with Chrony and that would resolve the cc_ntp testcase failures I'm seeing now. | 18:12 |
| rharper | minimal: I'm in favor of supporting both; | 18:12 |
| minimal | with the present code in my PR if you specify "pools" in the userdata it'll be ignored when the template is processed. However if you specify "pools" but don't specify "servers" then you'll not have any server entries in the resultant config file. If userdata doesn't specifiy either then the code will place default server lines in the config ok. | 18:18 |
| rharper | Yes; I think the implementation is fine, but when user-data is not processed as expected (but not fatal) I'm in favor of logging at least a warning; | 18:19 |
| rharper | this community goes back and forth between being resilient, not being overly noisy in logs (we're already really noisy) but indicating to users (via the log at least) that cloud-init encountered something it off the normal path | 18:20 |
| rharper | minimal: I think if we also in the docs for cc_ntp express that alpine ntp is busybox based and only supports servers (not pool) and indicates that it will enable default servers if no servers are present (and pools are ignored) that should be good enough; | 18:21 |
| rharper | even suggesting that if you want/need pools to consider chrony over ntp in your image | 18:21 |
| minimal | I'll have a look at the ntp testcases again to see how each it'd be to insert a "if distro != 'alpine':" clause for the ntp tests | 18:25 |
| minimal | Re: the other issue - using reRoot had given some partial progress, looks like the next step to fix is that I need to create the "/etc/apk" directory structure inside the new_root so that the module can actually create the file within it | 18:26 |
| rharper | minimal: yeah, you can util.ensure_dir(path) | 18:26 |
| minimal | rharper: Thanks for your help! - got the cc_apk_repositories test_handler passing all the tests now, just need to squash and push the commit. | 19:01 |
| minimal | rharper: I'll now have a look at tweaking the cc_ntp tests to treat Alpine + Busybox NTP as a special case | 19:01 |
| rharper | minimal: nice! | 19:09 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!