[10:42] rharper, This is Vijay from IBM, regarding the bug LP: 1893770. I have uploaded cloudinit log. Do you have any suggestion here? What should be the best approach do address this? [10:42] Launchpad bug 1893770 in cloud-init "Cloudinit resets network scripts to default configuration DHCP once Config Drive is removed after sometime" [Undecided,Incomplete] https://launchpad.net/bugs/1893770 [16:21] @Odd_Bloke I tagged you in this PR to follow up on our conversation last week. You said you had a local commit that could deal with the obj.pkl issue [16:21] https://github.com/canonical/cloud-init/pull/591 [16:21] AnhVoMSFT: Aha, thanks for the reminder, let me get that into shape. [16:26] AnhVoMSFT: To be clear, though, I don't think that needs to block #591 landing, so if(/once) rharper is happy then I think we're good to land? [16:26] vijayendra: thanks for the logs, I updated the bug [16:27] Odd_Bloke: AnhVoMSFT: I can review later today [16:29] sure - I thought you wanted to put that commit in the PR as well, but that can land separately it's all good. Thanks @Odd_Bloke @rharper [17:15] sometimes, after reading thru rharper's reviews, i don't think he knows happiness… [18:22] meena: =( [18:23] rharper: it wasn't meant to be *such* harsh criticism, more like, lighthearted jab [18:25] no worries; it's worth thinking about tone; so thank you for the nudge [18:26] rharper: tone is okay too… it's just often sooooooooo much [18:26] rharper: granted, the PRs you review are also loooooooooong too [18:26] * meena has lost much of … loooooooooong brain function… with child. [18:27] I see; I need a tl;dr read on for verbose details [18:27] like, all the things you demand are extremely reasonable, and very insightful (my python-fu is not as strong yet as I'd like it to be) [18:29] speaking of insightful comments…, anyone here got any clue as to why my tests are failing since i added @abstractmethod here: https://github.com/canonical/cloud-init/pull/588 [18:35] meena: I've not seen that sort of error; but maybe it has to do with how the test case is importing the class ? [18:41] rharper: dunno, it's not even the thing that's mocked. [18:47] meena: ah, in that file, cloudinit/distros/tests/test_networking.py the TestNetworking class, did not *define* the abstract method the class now requires an implementation of [18:48] you've said, all objects must define this method since the base class does not provide an implementation [18:48] well, it's distro based… [18:48] but not in that test [18:48] it's using a mock TestNetworking class [18:50] indeed, indeed. [18:50] I think you can create a generate_fallback method which calls the superclass method [18:51] if that's what's needed (or create an NotImplemented like the other methods) [18:53] i'll do that, but you'll have to explain how raise NotImplemented is better than whatever @abstractmethod does [18:54] the abstractmethod requires that subclasses provide an implementation [18:54] it's for creating an interface; what methods on these objects are *expected* to be present [18:55] you can skip this; but coders want to implement a subclass of an interface usually like to know which methods the *must* implement, versus overriding for their own purposes; [18:55] i think i'm just curious about the mechanism underneath. What is it, if not a thrown exception? [18:56] it works. [18:56] it raises as TypeError (which is also an exception) [18:57] right… that is what i'm seeing in the tests, now that makes sense. [18:58] and why is our mock class happy with the NotImplemented exception [18:59] the exception is coming from the abc.abstractclass decorator . it doesn't *care* about the implementation of the abstract method, only that it _is_ present [18:59] it's your subclass [19:01] aye [19:02] rharper: thanks. pushed. [19:08] sure [19:15] You get the TypeError at instantiation time, whereas the NotImplementedError would be raised iff you called the unimplemented method. [19:15] aye [19:15] that makes sense. [19:16] so, question of tatste: Do i start replacing calls of distro.generate_fallback_config() with distro.networking.generate_fallback_config()? or do i leave those intact? [19:16] @rharper - sent another iteration to fix up the docstring [19:18] meena: I would expect that we could replace them all and then remove distro.generate_fallback_config, but I think we could also not and it wouldn't matter too much. [19:18] The important thing is that the per-distro networking class is being used, which is achieved. [19:20] Odd_Bloke: i think i'm mostly asking because of the current refactor of adding find_fallback_nic() being modified [19:20] which then in turns means that generate_fallback_config() must be, and so it's easier to only modify inplace [19:21] in one place [19:22] meena: I'm not sure I follow; if distro.generate_fallback_config is calling distro.networking.generate_fallback_config, then don't you just have one place to modify? [19:23] Odd_Bloke: everything calling the various net.generate_fallback_config() and the various distro.generate_fallback_config() is currently calling it without parameters. [19:24] also, why is this failing? https://github.com/canonical/cloud-init/actions/runs/305025962 [19:25] https://github.com/canonical/cloud-init/actions/runs/305048953 failed too [19:25] maybe some thing on the Canonical side for the CLA [19:25] https://www.githubstatus.com/incidents/nq2s4gt7xs1w [19:26] i was worried for a second about… things. [19:26] The GitHub Action only examines the files in-tree, we intentionally avoided adding a dependency on another service (at the cost of having to maintain those lists manually, ofc). [19:27] And because there are changes that don't require CLA signature (e.g. docs), it's not _required_ to land things, so provided Travis results are still getting through OK, we should still be able to land things. [19:30] AnhVoMSFT: ok [19:30] anyway, i feel like changing things to call distro.networking.foo() instead of yet another wrapper should make things easier [19:31] but then i have to fix a lot of tests… [19:32] meena: Yeah, I think ultimately it's cleaner, but it's not necessarily more correct, so it's a question of whether that's how you want to spend your time. :p [19:38] Odd_Bloke: what i want to spend my time with is sleep, most of all [19:38] Odd_Bloke: here's the failure when i change the call in cloudinit/stages.py [19:39] https://gist.github.com/igalic/7c6648aebe4b384e0138060b1f2c5d2f [19:39] here [19:41] meena: `distro.generate_fallback_config = ...` is "mocking" and will no longer be called. [19:50] Odd_Bloke: how do i add the networking leayer in between there? [19:53] lol, it already exists [20:11] okay, so, the "only" thing left is replace the calls to net.generate_fallback_config() now… [20:11] AnhVoMSFT: rharper: https://github.com/canonical/cloud-init/pull/609 is the bugfix we discussed related to #591. [20:11] and the hardest part there will be Azure [20:12] hrm, apparently, that's actually the only part that needs fixing [20:12] the rest is just unit tests of net.generate_fallback_config() — which maybe just be moved [20:20] yeaaaahh, fixing Azure and its tests is gonna be great fun [20:21] great fun for another day [21:21] okay, this is pretty much how far i'm able to carry that https://github.com/canonical/cloud-init/pull/588 pr without hand-holding or other forms of help [21:52] rharper: https://github.com/canonical/cloud-init/pull/591#discussion_r504176429 what's the advantage of doing this in test code? [21:56] Odd_Bloke, what is "unpickled objects"? [22:01] https://github.com/canonical/cloud-init/pull/588/commits/0af627029237ea8b362c20c57358e411eb65c0d6 this can be done with that mock call! [22:02] Odd_Bloke, same thing https://stackoverflow.com/questions/9757299/python-testing-an-abstract-base-class/28738073#28738073 right?