| Chipaca | good morning! | 09:26 |
|---|---|---|
| niemeyer | Moin moin! | 09:34 |
| Chipaca | jam: niemeyer: have you guys talked about the testing pr any more than what's on github? | 09:39 |
| niemeyer | We did.. but I think we still misunderstood each other and need to sync again | 09:42 |
| niemeyer | The enable events method with a charm instance was unexpected to me at least | 09:43 |
| Chipaca | niemeyer: I think reading the __init__ method, the way charm is passed in changes | 09:44 |
| Chipaca | niemeyer: ie in initialize(charm) instead of Harness(charm) | 09:44 |
| Chipaca | bah, MyCharm* | 09:45 |
| Chipaca | i think that's making things too granular ( → harder to use) | 09:46 |
| Chipaca | and would prefer just __init__() doing the initalization work (could just call _initialize) | 09:46 |
| niemeyer | I still don't get it.. wasn't the create_harness method receiving a charm type? Why did it change? | 09:47 |
| Chipaca | I think it was but I might misremember | 09:47 |
| Chipaca | https://github.com/canonical/operator/pull/146/commits/c2639df120efe2c56235511c814e71644dd640d0 is where that changed | 09:52 |
| Chipaca | and I think on its own it makes sense | 09:52 |
| Chipaca | that is: don't pass in the charm class, then do all the setup you need, and then initialize | 09:53 |
| Chipaca | thus initialize kicks off events | 09:53 |
| Chipaca | I don't think it makes sense in conjunction with explicit enable/disable methods | 09:53 |
| Chipaca | especially with the default of enabled | 09:53 |
| Chipaca | niemeyer: this also explains why enable takes the charm :-) | 09:53 |
| Chipaca | in fact I think I like the approach of build without the charm, set up, then initialize the charm to start handling events; it feels very natural | 09:54 |
| Chipaca | at this point enable/disable_events are handles for doing fancier stuff (but I don't see the need for passing in the charm to enable, beyond "disable sets it to None was the easiest way of disabling events" | 09:55 |
| Chipaca | ) | 09:55 |
| niemeyer | Chipaca: I don't see how it makes sense.. | 09:57 |
| niemeyer | Chipaca: The charm isn't created by the developer in practice anyway | 09:57 |
| niemeyer | Sorry, created is not a good term here | 09:57 |
| niemeyer | I mean it's not instantiated | 09:57 |
| Chipaca | niemeyer: i assume the no-make-sense bit is enable_events taking a charm instance? | 09:58 |
| niemeyer | Was talking about the argument itself | 09:58 |
| niemeyer | But yes, that's where the conversation started | 09:59 |
| niemeyer | Also, disabling resetting it | 09:59 |
| Chipaca | niemeyer: sorry, which argument? | 09:59 |
| * Chipaca now lost | 09:59 | |
| niemeyer | Chipaca: The conversation we're having just now? | 09:59 |
| Chipaca | niemeyer: ah :) | 09:59 |
| Chipaca | i thought you meant argument to the method | 09:59 |
| niemeyer | Ah, no, sorry | 09:59 |
| Chipaca | niemeyer: yes, agreed, having disable/enable fiddle with the charm on the handler is weird / wrong | 09:59 |
| Chipaca | niemeyer: but the constructor without the charm class, and then explicit initialize, feels alright to me | 10:01 |
| Chipaca | niemeyer: it means in most cases you won't need to poke at enable/disable at all | 10:01 |
| Chipaca | if we agree on that, then we can look at how to make enable/disable not fiddle with the charm instance | 10:02 |
| niemeyer | I had to jump into a call at 10, but we can keep going in a bit | 10:05 |
| Chipaca | if we don't, then the other coherent thing is to not have initialize, and just enable/disable, that makes the need to pass the charm instance in go away | 10:06 |
| Chipaca | the code to not pass in the charm instance to enable/disbale is probably the same in both cases, and it's not pretty but not terrible either (unless i miss something) | 10:07 |
| Chipaca | but it's all hidden from the dev, so that's alright | 10:07 |
| Chipaca | the passing it in is a wart exposed to the dev, or at least it feels like it --- if there was a non-warty reason i don't see it | 10:07 |
| Chipaca | no probs, i have things i need to read :) | 10:08 |
| * Chipaca goes read | 10:08 | |
| niemeyer | > 10:01:28 niemeyer: but the constructor without the charm class, and then explicit initialize, feels alright to me | 10:36 |
| niemeyer | If that's true I must be missing something about how we'll use it | 10:36 |
| niemeyer | We went from "this really needs to return the two arguments because that's what we're testing" to "the charm can be created separately and needs to be fed manually by calling some method" | 10:37 |
| Chipaca | niemeyer: it's initialize(MyCharm), not initialize(charm), right? | 10:38 |
| niemeyer | What's "initialize"? | 10:38 |
| Chipaca | niemeyer: instantiate the charm and plug it in to the framework? | 10:39 |
| niemeyer | Sorry, I'm completely lost | 10:39 |
| Chipaca | niemeyer: https://github.com/canonical/operator/pull/146/commits/c2639df120efe2c56235511c814e71644dd640d0#diff-6964ca15c08db5ba857f6517369815d9R72 | 10:39 |
| niemeyer | Yes, I don't get any of that | 10:40 |
| niemeyer | In my mind it was "harness = Harness(MyCharm)" | 10:40 |
| Chipaca | niemeyer: also with some yaml, right? | 10:41 |
| niemeyer | Optionally I think, but yes | 10:42 |
| Chipaca | niemeyer: and that does a couple of things: creates a framework that has a model with a testing backend, and instantiates the charm class hooked into that framework | 10:45 |
| niemeyer | Yeah, it does all the boring boilerplate | 10:45 |
| Chipaca | niemeyer: initialize() splits out the second of those two things into an explicit method | 10:45 |
| niemeyer | Because....? | 10:45 |
| Chipaca | niemeyer: because then you can tweak the model by adding relationships and whatnot, without having the charm hooked up to it yet | 10:46 |
| Chipaca | ie without handling events | 10:46 |
| niemeyer | Chipaca: Which events? There can't possibly be any events to consume at this stage, right? | 10:47 |
| Chipaca | niemeyer: if you do: harness.add_relation(...), and the charm was already plugged in, it'll get the relation_created (say) | 10:48 |
| Chipaca | niemeyer: or add_relation_unit() would trigger relation_joined | 10:48 |
| Chipaca | etc | 10:48 |
| niemeyer | Chipaca: That's intended.. to disable that behavior you'd call disable_events() | 10:48 |
| niemeyer | Which perhaps should really be called disable_hooks, actually, since we don't want to disable internal events I think | 10:49 |
| niemeyer | but that's an aside | 10:49 |
| Chipaca | niemeyer: right, having initialize() means that the thing starts with events disabled, and then you hook things up and they're enabled, in a natural way | 10:50 |
| Chipaca | niemeyer: at that point we could probably not have enable/disable at all, unless we found some advanced scenarios where they made sense | 10:52 |
| niemeyer | Chipaca: It sounds to me like you're trying to retrofit the design into what we have in the PR | 10:52 |
| niemeyer | Chipaca: The PR includes initialize, disable_events, and enable_events | 10:52 |
| Chipaca | heh | 10:52 |
| Chipaca | maybe | 10:53 |
| niemeyer | Chipaca: And __init__, which is a prefix of "initialize" :) | 10:53 |
| niemeyer | Chipaca: So the rationale you just provided is at least not the reason why it ended up like this | 10:53 |
| Chipaca | to be clear, I _don't_ like passing the charm into enable | 10:53 |
| niemeyer | Chipaca: And when you call update_config, will we run hooks or not? | 10:54 |
| Chipaca | niemeyer: when? | 10:54 |
| Chipaca | :) | 10:54 |
| Chipaca | seriously: in which scenario (with initialize? without?), and at what point (after enable/disable/initialize/frob/...?) | 10:55 |
| niemeyer | You pick it.. do you want to fire hooks every time after your preferred point of initialization? | 10:56 |
| niemeyer | Can you foresee reasons not to? | 10:56 |
| niemeyer | How do you handle it? | 10:56 |
| Chipaca | hah, 1 sec | 10:56 |
| Chipaca | niemeyer: jam already did https://github.com/jameinel/operator/pull/3/files fwiw | 10:56 |
| niemeyer | It would be nice to have jam with us at some point.. is he working today? | 10:56 |
| Chipaca | he is, or at least, he just merged master | 10:57 |
| niemeyer | Why are we both having this conversation without us? Is this channel a group chat or not? | 10:57 |
| niemeyer | I don't care if we use IRC, or Telegram, or telnet.. but we need to have people communicating if we expect this thing to work at all | 10:58 |
| Chipaca | I shall enquire | 10:58 |
| jam | I am around, I just don't sit with IRC in my view | 10:59 |
| niemeyer | jam, Chipaca: I don't care about which chat you guys use, but you need notifications active on the channel where you team works | 10:59 |
| Chipaca | I should've pinged jam earlier in this, is all | 11:00 |
| jam | I think I was probably making coffee when you pinged earlier, my apologies. | 11:00 |
| niemeyer | Chipaca: No, not really.. this is the team channel.. it won't work if you need to ping people to pay attention whenever there's a conversation | 11:01 |
| jam | niemeyer: Chipaca: so as I mentioned in my PR response, decoupling the charm being instantiated from the Harness felt like a nicer separation of concerns. Especially for code that wanted to test changes to the model without events, it felt much more natural to not have the charm created yet | 11:21 |
| jam | (especially, for test_model.py, but I realize that is a bit of a special case of testing the framework itself) | 11:22 |
| jam | *some* sort of helper function is currently necessary because of the need to have unique Class definitions, which I will be looking at next. | 11:23 |
| niemeyer | Isn't the charm the entire thing the harness is testing, with extreme coupling so that you can manipulate its fake backend model to arbitrary degrees? | 11:23 |
| niemeyer | Isn't every emit_foo() method going to be injecting events into that charm? | 11:23 |
| niemeyer | Every other updating method effectively going to be invoking things into it? | 11:23 |
| jam | events are definitely coupled to the charm instance, as that is the only way to generate hook events. | 11:25 |
| niemeyer | What is the harness doing that it *not* coupled to the charm? | 11:25 |
| jam | Though the fact that we've stated "we may not want to have events emitted yet" felt like a case of setting up the Model/Framework without a Charm, and then instantiating the charm once the setup was done felt quite natural | 11:25 |
| jam | All of the state-of-the-model is underneath charm. Such that the charm can go look at it, but doesn't require a charm to exist for the model to be updated. | 11:26 |
| Chipaca | to me, the question goes back to how we expect this to be used more | 11:29 |
| niemeyer | That's fine.. it's what Chipaca was suggesting above. We can postpone the creation of the charm and have an explicit method for that. It doesn't change everything else we discussed, though: | 11:29 |
| niemeyer | - We shouldn't make people play puzzle on every test to assemble the harness | 11:30 |
| niemeyer | - There's no reason for disable_event/enable_event to take a charm and kill the internal charm instance | 11:30 |
| niemeyer | - We may need to stop emitting hooks and make that manual so that people can try to trigger particular cases | 11:30 |
| niemeyer | - The default case is supposed to be straightforward | 11:31 |
| niemeyer | Having something like a harness.start() method would address Chipaca's suggestion.. but we still need to address everything else | 11:32 |
| Chipaca | niemeyer: what's the puzzle you refer to in the first point? | 11:32 |
| niemeyer | Chipaca: "... to assemble the harness"? | 11:33 |
| jam | so the current proposed syntax if we rename 'initialize' to 'start' and change enable/disable to use a boolean would be: | 11:36 |
| jam | https://paste.ubuntu.com/p/s4QcKGX3Xr/ | 11:36 |
| jam | niemeyer: Chipaca ^^ | 11:36 |
| Chipaca | sorry, having to check on the boys' home work | 11:36 |
| Chipaca | back now | 11:36 |
| jam | we have to have the charm meta to initialize a Model | 11:37 |
| niemeyer | Let me quickly draft something | 11:37 |
| * Chipaca waitws | 11:38 | |
| * Chipaca waits, also | 11:38 | |
| jam | if you have a different syntax that feels more natural, having examples are good. I've had these in tests, etc, but I'm happy to pull them out explicitly and discuss them. | 11:38 |
| niemeyer | https://paste.ubuntu.com/p/XfjJ95NfB4/ | 11:40 |
| Chipaca | niemeyer: could make the yaml an optional arg on the constructor | 11:44 |
| Chipaca | instead of having it as a method (because then you have to think about what happens if you call it after you've started or whatever) | 11:44 |
| niemeyer | Chipaca: You could, but this is usually long string | 11:44 |
| niemeyer | Chipaca: harness = Harness(Charm) feels solid | 11:45 |
| niemeyer | Chipaca: You don't have to think if you don't allow it to take place | 11:45 |
| jam | niemeyer: so one thing that I was using this for testing is testing components that aren't themselves a charm. And we don't have a metadata.yaml for CharmBase | 11:45 |
| niemeyer | jam: I'm a bit lost.. CharmBase is the base class for charms.. charms have metadata.. ? | 11:48 |
| jam | niemeyer: if I'm testing MysqlClient, it doesn't have its own charm. it will be used by several charms, and does consume events. | 11:48 |
| jam | we need 'name' from metadata.yaml | 11:49 |
| Chipaca | i need to step out but will catch up when i return | 11:50 |
| niemeyer | jam: How does that change things in the context above? | 11:50 |
| jam | niemeyer: so if you just pass CharmBase, there isn't a metadata.yaml, even though it does have defined events | 11:51 |
| niemeyer | jam: We can just have a stock metadata.yaml, but that won't have any endpoints that will be useful for MysqlClient.. | 11:51 |
| niemeyer | jam: The only reasonable answer I can tihnk of is you actually define a trivial test charm and metadata to test it.. but I'm not sure if you're thinking of specific issues? | 11:52 |
| jam | so I've been going with explicit metadata for each instantiation, which does have the nice property that you declare your needs right before you start testing them. | 11:54 |
| jam | but I can understand when testing a real charm, that ends up copying the metadata you've already had to declare | 11:54 |
| jam | niemeyer: does an optional __init__ parameter seem ok? I'm not sure about changing metadata late with set_meta() | 11:55 |
| niemeyer | jam: Both of these cases seem covered in the example paste I provided above.. so I'm still not entirely sure if you're trying to point something that I'm completely oblivious to | 11:55 |
| niemeyer | jam: What do you mean "late"? Isn't the entire point of the start() method to enable a setup phase? | 11:55 |
| jam | niemeyer: so start() for *me* was to delay when the Charm was instantiated, but to still have a Model that you can populate. If you can't do anything before start() then there is no reason to decouple it from __init__ | 11:57 |
| niemeyer | jam: Exactly.. that's what I'm saying as well | 11:58 |
| jam | Model can't be instantiated before Meta is finalized, as it needs it for things like "what relations will I know about". So you can't have 'set_meta' to set it without replacing the Model. | 11:58 |
| jam | niemeyer: so with the code that I proposed, you can perfectly populate Model with relation data, config data, etc, all without a Charm. | 11:59 |
| niemeyer | I see | 11:59 |
| jam | and then when you *do* create a Charm, it has access to a populated Model. | 11:59 |
| niemeyer | jam: Do you populate the model, or do you populate the backend? | 12:00 |
| jam | niemeyer: the backend. | 12:00 |
| niemeyer | jam: The backend needs meta? | 12:00 |
| jam | niemeyer: _ModelBackend doesn't need meta, but you pass in the backend to Model in its __init__ and Model does need both Backend and Meta. | 12:01 |
| niemeyer | jam: If you populate the backend, and it doesn't need meta, can we create the model at start time? | 12:01 |
| jam | niemeyer: and Framework which is a base for everything also needs Model, but that, again, could be constructed late. | 12:02 |
| jam | niemeyer: so yes, we could fix Meta, create Model and Framework and Charm during start | 12:02 |
| jam | and then have set_meta() refuse if Model has been created. | 12:03 |
| niemeyer | start() is a bad name as we have a hook with that name.. begin(), bake(), setup() | 12:04 |
| niemeyer | setup is also a bad name as that's what we've been doing before we call the method | 12:05 |
| niemeyer | .begin() is likely the better choice of those, as it has the feeling of "alright, we're ready and will start doing things now" | 12:06 |
| jam | initialize(Charm), instantiate(Charm), create(Charm), begin(Charm) | 12:07 |
| jam | charm = harness.begin(Charm) | 12:07 |
| jam | doesn't quite read naturally to me, but I could live with it. I do like it meaning the 'start of testing that I'm going to be doing' | 12:08 |
| niemeyer | harness = Harness(Charm) | 12:08 |
| niemeyer | harness.begin() | 12:08 |
| niemeyer | "harness = Harness(Charm)" is what we had agreed on the sprint, and still feels right to me | 12:08 |
| jam | ok | 12:09 |
| niemeyer | We can cover the reasons again once more, but let's do that in the call | 12:09 |
| niemeyer | I'm going to step out for lunch now | 12:09 |
| jam | np | 12:10 |
| * Chipaca running late | 13:31 | |
| jam | Chipaca: niemeyer: patch updated. I went with meta as an __init__ parameter instead of set_meta because we need it for a default unit_name. There could be other ways around that (having the user supply an optional unit_name might be better). | 15:22 |
| jam | I did implement the 'look for metadata.yaml if not supplied', and added a test for it. | 15:22 |
| jam | It is a test that is 'impure' vs the rest of the tests. (Everything else is just python objects, but that involves manipulating the filesystem, and importlib stuff.) | 15:23 |
| jam | Anyway, EOD for me. Have a good afternoon/evening. | 15:23 |
| niemeyer | Thanks, have a good one | 15:23 |
| * Chipaca soft-EODs (need to pop back later to wrap up some things, but dinner now) | 18:33 | |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!