[10:42] <vijayendra> 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?
[16:21] <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 issue
[16:21] <AnhVoMSFT> https://github.com/canonical/cloud-init/pull/591
[16:21] <Odd_Bloke> AnhVoMSFT: Aha, thanks for the reminder, let me get that into shape.
[16:26] <Odd_Bloke> 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] <rharper> vijayendra: thanks for the logs, I updated the bug
[16:27] <rharper> Odd_Bloke: AnhVoMSFT: I can review later today
[16:29] <AnhVoMSFT> 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] <meena> sometimes, after reading thru rharper's reviews, i don't think he knows happiness…
[18:22] <rharper> meena: =(
[18:23] <meena> rharper: it wasn't meant to be *such* harsh criticism, more like, lighthearted jab
[18:25] <rharper> no worries;  it's worth thinking about tone; so thank you for the nudge
[18:26] <meena> rharper: tone is okay too… it's just often sooooooooo much
[18:26] <meena> rharper: granted, the PRs you review are also loooooooooong too
[18:26]  * meena has lost much of … loooooooooong brain function… with child.
[18:27] <rharper> I see;  I need a tl;dr  read on for verbose details
[18:27] <meena> 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] <meena> 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] <rharper> 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] <meena> rharper: dunno, it's not even the thing that's mocked.
[18:47] <rharper> 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] <rharper> you've said, all objects must define this method since the base class does not provide an implementation
[18:48] <meena> well, it's distro based…
[18:48] <rharper> but not in that test
[18:48] <rharper> it's using a mock TestNetworking class
[18:50] <meena> indeed, indeed.
[18:50] <rharper> I think you can create a generate_fallback method which calls the superclass method
[18:51] <rharper> if that's what's needed (or create an NotImplemented like the other methods)
[18:53] <meena> i'll do that, but you'll have to explain how raise NotImplemented is better than whatever @abstractmethod does
[18:54] <rharper> the abstractmethod requires that subclasses provide an implementation
[18:54] <rharper> it's for creating an interface;  what methods on these objects are *expected* to be present
[18:55] <rharper> 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] <meena> i think i'm just curious about the mechanism underneath. What is it, if not a thrown exception?
[18:56] <meena> it works.
[18:56] <rharper> it raises as  TypeError (which is also an exception)
[18:57] <meena> right… that is what i'm seeing in the tests, now that makes sense.
[18:58] <meena> and why is our mock class happy with the NotImplemented exception
[18:59] <rharper> 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] <rharper> it's your subclass
[19:01] <meena> aye
[19:02] <meena> rharper: thanks. pushed.
[19:08] <rharper> sure
[19:15] <Odd_Bloke> You get the TypeError at instantiation time, whereas the NotImplementedError would be raised iff you called the unimplemented method.
[19:15] <meena> aye
[19:15] <meena> that makes sense.
[19:16] <meena> 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] <AnhVoMSFT> @rharper - sent another iteration to fix up the docstring
[19:18] <Odd_Bloke> 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] <Odd_Bloke> The important thing is that the per-distro networking class is being used, which is achieved.
[19:20] <meena> Odd_Bloke: i think i'm mostly asking because of the current refactor of adding find_fallback_nic() being modified
[19:20] <meena> which then in turns means that generate_fallback_config() must be, and so it's easier to only modify inplace
[19:21] <meena> in one place
[19:22] <Odd_Bloke> 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] <meena> 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] <meena> also, why is this failing? https://github.com/canonical/cloud-init/actions/runs/305025962
[19:25] <AnhVoMSFT> https://github.com/canonical/cloud-init/actions/runs/305048953 failed too
[19:25] <AnhVoMSFT> maybe some thing on the Canonical side for the CLA
[19:25] <Odd_Bloke> https://www.githubstatus.com/incidents/nq2s4gt7xs1w
[19:26] <meena> i was worried for a second about… things.
[19:26] <Odd_Bloke> 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] <Odd_Bloke> 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] <rharper> AnhVoMSFT: ok
[19:30] <meena> anyway, i feel like changing things to call distro.networking.foo() instead of yet another wrapper should make things easier
[19:31] <meena> but then i have to fix a lot of tests…
[19:32] <Odd_Bloke> 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] <meena> Odd_Bloke: what i want to spend my time with is sleep, most of all
[19:38] <meena> Odd_Bloke: here's the failure when i change the call in cloudinit/stages.py
[19:39] <meena> https://gist.github.com/igalic/7c6648aebe4b384e0138060b1f2c5d2f
[19:39] <meena> here
[19:41] <Odd_Bloke> meena: `distro.generate_fallback_config = ...` is "mocking" and will no longer be called.
[19:50] <meena> Odd_Bloke: how do i add the networking leayer in between there?
[19:53] <meena> lol, it already exists
[20:11] <meena> okay, so, the "only" thing left is replace the calls to net.generate_fallback_config() now…
[20:11] <Odd_Bloke> AnhVoMSFT: rharper: https://github.com/canonical/cloud-init/pull/609 is the bugfix we discussed related to #591.
[20:11] <meena> and the hardest part there will be Azure
[20:12] <meena> hrm, apparently, that's actually the only part that needs fixing
[20:12] <meena> the rest is just unit tests of net.generate_fallback_config() — which maybe just be moved
[20:20] <meena> yeaaaahh, fixing Azure and its tests is gonna be great fun
[20:21] <meena> great fun for another day
[21:21] <meena> 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] <meena> rharper: https://github.com/canonical/cloud-init/pull/591#discussion_r504176429 what's the advantage of doing this in test code?
[21:56] <meena> Odd_Bloke, what is "unpickled objects"?
[22:01] <meena> https://github.com/canonical/cloud-init/pull/588/commits/0af627029237ea8b362c20c57358e411eb65c0d6 this can be done with that mock call!
[22:02] <meena> Odd_Bloke, same thing https://stackoverflow.com/questions/9757299/python-testing-an-abstract-base-class/28738073#28738073 right?