| 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? | 10:42 |
|---|---|---|
| ubot5 | 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 | 10: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 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:21 |
| 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:26 |
| rharper | Odd_Bloke: AnhVoMSFT: I can review later today | 16:27 |
| 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 | 16:29 |
| meena | sometimes, after reading thru rharper's reviews, i don't think he knows happiness… | 17:15 |
| rharper | meena: =( | 18:22 |
| meena | rharper: it wasn't meant to be *such* harsh criticism, more like, lighthearted jab | 18:23 |
| rharper | no worries; it's worth thinking about tone; so thank you for the nudge | 18:25 |
| 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:26 | |
| 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:27 |
| 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:29 |
| 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:35 |
| meena | rharper: dunno, it's not even the thing that's mocked. | 18:41 |
| 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:47 |
| 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:48 |
| meena | indeed, indeed. | 18:50 |
| rharper | I think you can create a generate_fallback method which calls the superclass method | 18:50 |
| rharper | if that's what's needed (or create an NotImplemented like the other methods) | 18:51 |
| meena | i'll do that, but you'll have to explain how raise NotImplemented is better than whatever @abstractmethod does | 18:53 |
| 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:54 |
| 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:55 |
| meena | it works. | 18:56 |
| rharper | it raises as TypeError (which is also an exception) | 18:56 |
| meena | right… that is what i'm seeing in the tests, now that makes sense. | 18:57 |
| meena | and why is our mock class happy with the NotImplemented exception | 18:58 |
| 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 | 18:59 |
| meena | aye | 19:01 |
| meena | rharper: thanks. pushed. | 19:02 |
| rharper | sure | 19:08 |
| 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:15 |
| 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:16 |
| 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:18 |
| 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:20 |
| meena | in one place | 19:21 |
| 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:22 |
| 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:23 |
| meena | also, why is this failing? https://github.com/canonical/cloud-init/actions/runs/305025962 | 19:24 |
| 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:25 |
| 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:26 |
| 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:27 |
| 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:30 |
| meena | but then i have to fix a lot of tests… | 19:31 |
| 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:32 |
| 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:38 |
| meena | https://gist.github.com/igalic/7c6648aebe4b384e0138060b1f2c5d2f | 19:39 |
| meena | here | 19:39 |
| Odd_Bloke | meena: `distro.generate_fallback_config = ...` is "mocking" and will no longer be called. | 19:41 |
| meena | Odd_Bloke: how do i add the networking leayer in between there? | 19:50 |
| meena | lol, it already exists | 19:53 |
| 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:11 |
| 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:12 |
| meena | yeaaaahh, fixing Azure and its tests is gonna be great fun | 20:20 |
| meena | great fun for another day | 20: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:21 |
| meena | rharper: https://github.com/canonical/cloud-init/pull/591#discussion_r504176429 what's the advantage of doing this in test code? | 21:52 |
| meena | Odd_Bloke, what is "unpickled objects"? | 21:56 |
| meena | https://github.com/canonical/cloud-init/pull/588/commits/0af627029237ea8b362c20c57358e411eb65c0d6 this can be done with that mock call! | 22:01 |
| meena | Odd_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!