vijayendrarharper, 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
ubot5Launchpad 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/189377010:42
AnhVoMSFT@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 issue16:21
Odd_BlokeAnhVoMSFT: Aha, thanks for the reminder, let me get that into shape.16:21
Odd_BlokeAnhVoMSFT: 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
rharpervijayendra: thanks for the logs, I updated the bug16:26
rharperOdd_Bloke: AnhVoMSFT: I can review later today16:27
AnhVoMSFTsure - 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 @rharper16:29
meenasometimes, after reading thru rharper's reviews, i don't think he knows happiness…17:15
rharpermeena: =(18:22
meenarharper: it wasn't meant to be *such* harsh criticism, more like, lighthearted jab18:23
rharperno worries;  it's worth thinking about tone; so thank you for the nudge18:25
meenarharper: tone is okay too… it's just often sooooooooo much18:26
meenarharper: granted, the PRs you review are also loooooooooong too18:26
* meena has lost much of … loooooooooong brain function… with child.18:26
rharperI see;  I need a tl;dr  read on for verbose details18:27
meenalike, 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:27
meenaspeaking 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/58818:29
rharpermeena: I've not seen that sort of error; but maybe it has to do with how the test case is importing the class ?18:35
meenarharper: dunno, it's not even the thing that's mocked.18:41
rharpermeena: 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 of18:47
rharperyou've said, all objects must define this method since the base class does not provide an implementation18:48
meenawell, it's distro based…18:48
rharperbut not in that test18:48
rharperit's using a mock TestNetworking class18:48
meenaindeed, indeed.18:50
rharperI think you can create a generate_fallback method which calls the superclass method18:50
rharperif that's what's needed (or create an NotImplemented like the other methods)18:51
meenai'll do that, but you'll have to explain how raise NotImplemented is better than whatever @abstractmethod does18:53
rharperthe abstractmethod requires that subclasses provide an implementation18:54
rharperit's for creating an interface;  what methods on these objects are *expected* to be present18:54
rharperyou 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
meenai think i'm just curious about the mechanism underneath. What is it, if not a thrown exception?18:55
meenait works.18:56
rharperit raises as  TypeError (which is also an exception)18:56
meenaright… that is what i'm seeing in the tests, now that makes sense.18:57
meenaand why is our mock class happy with the NotImplemented exception18:58
rharperthe exception is coming from the abc.abstractclass decorator .  it doesn't *care* about the implementation of the abstract method, only that it _is_ present18:59
rharperit's your subclass18:59
meenarharper: thanks. pushed.19:02
Odd_BlokeYou get the TypeError at instantiation time, whereas the NotImplementedError would be raised iff you called the unimplemented method.19:15
meenathat makes sense.19:15
meenaso, 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
AnhVoMSFT@rharper - sent another iteration to fix up the docstring19:16
Odd_Blokemeena: 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
Odd_BlokeThe important thing is that the per-distro networking class is being used, which is achieved.19:18
meenaOdd_Bloke: i think i'm mostly asking because of the current refactor of adding find_fallback_nic() being modified19:20
meenawhich then in turns means that generate_fallback_config() must be, and so it's easier to only modify inplace19:20
meenain one place19:21
Odd_Blokemeena: 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:22
meenaOdd_Bloke: everything calling the various net.generate_fallback_config() and the various distro.generate_fallback_config() is currently calling it without parameters.19:23
meenaalso, why is this failing? https://github.com/canonical/cloud-init/actions/runs/30502596219:24
AnhVoMSFThttps://github.com/canonical/cloud-init/actions/runs/305048953 failed too19:25
AnhVoMSFTmaybe some thing on the Canonical side for the CLA19:25
meenai was worried for a second about… things.19:26
Odd_BlokeThe 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:26
Odd_BlokeAnd 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:27
rharperAnhVoMSFT: ok19:30
meenaanyway, i feel like changing things to call distro.networking.foo() instead of yet another wrapper should make things easier19:30
meenabut then i have to fix a lot of tests…19:31
Odd_Blokemeena: 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. :p19:32
meenaOdd_Bloke: what i want to spend my time with is sleep, most of all19:38
meenaOdd_Bloke: here's the failure when i change the call in cloudinit/stages.py19:38
Odd_Blokemeena: `distro.generate_fallback_config = ...` is "mocking" and will no longer be called.19:41
meenaOdd_Bloke: how do i add the networking leayer in between there?19:50
meenalol, it already exists19:53
meenaokay, so, the "only" thing left is replace the calls to net.generate_fallback_config() now…20:11
Odd_BlokeAnhVoMSFT: rharper: https://github.com/canonical/cloud-init/pull/609 is the bugfix we discussed related to #591.20:11
meenaand the hardest part there will be Azure20:11
meenahrm, apparently, that's actually the only part that needs fixing20:12
meenathe rest is just unit tests of net.generate_fallback_config() — which maybe just be moved20:12
meenayeaaaahh, fixing Azure and its tests is gonna be great fun20:20
meenagreat fun for another day20:21
meenaokay, 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 help21:21
meenarharper: https://github.com/canonical/cloud-init/pull/591#discussion_r504176429 what's the advantage of doing this in test code?21:52
meenaOdd_Bloke, what is "unpickled objects"?21:56
meenahttps://github.com/canonical/cloud-init/pull/588/commits/0af627029237ea8b362c20c57358e411eb65c0d6 this can be done with that mock call!22:01
meenaOdd_Bloke, same thing https://stackoverflow.com/questions/9757299/python-testing-an-abstract-base-class/28738073#28738073 right?22:02

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!