| Chipaca | good morning peeps! | 09:14 |
|---|---|---|
| niemeyer | Moin moin | 09:14 |
| niemeyer | Chipaca: Didn't get to do a full round of reviews yesterday.. thankfully friends/family called | 09:40 |
| niemeyer | Got through the harness one, though, and just replied to your first requested point above | 09:40 |
| Chipaca | niemeyer: glad you got some social interaction in :-) | 09:41 |
| Chipaca | i went out and had a belated birthday dinner with my uncle and my brother+sister-in-law | 09:42 |
| Chipaca | we tried not to cough too much at people | 09:42 |
| Chipaca | now i'm going to finish the HR training thing before they get upset | 09:43 |
| niemeyer | Replied to #178 as well | 10:08 |
| Dmitrii-Sh | Thanks for the replies on #157. I made a comment about the "default binding" term here https://github.com/canonical/operator/pull/157#discussion_r392150027 - I hope the example makes sense | 10:40 |
| niemeyer | Replied | 10:46 |
| Chipaca | i need to step out for a bit | 10:48 |
| * Chipaca realises he hasn't actually moved | 10:51 | |
| Facu | Muy buenos días a todos! | 11:24 |
| Chipaca | Facu: o/! | 11:26 |
| niemeyer | \o | 11:27 |
| niemeyer | I'm going to skip the standup today as I'm researching something for a call with Mark in a bit.. I've been reviewing and will be reviewing is my main action points | 11:42 |
| Facu | niemeyer, (no rush, just don't want to drop your comment from yesterday) -- "That DBAvailable event really wants me to make StoredState work there" <-- this means to provide to events the same mechanism of "store whatever you want, then access it later" for the charms, right? | 11:47 |
| niemeyer | Facu: I meant supporting a _stored = StoredState() attribute in events so people don't need to implement the methods | 11:48 |
| Facu | niemeyer, having "one mechanism to rule them all" sounds nice, not having to learn two things... however, Events may be simpler than that, we could have the responsability of spawn the same event using the original information automatically, no developer code needed | 11:48 |
| Facu | we could just "freeze" the events and melt them later... we want for Events to not carry around any extra information, right? | 11:52 |
| Facu | what about defining the events like this? https://pastebin.canonical.com/p/yzbpvNMKHW/ | 11:53 |
| niemeyer | Facu: The thinking behind StoredState is that it enables people to very clearly define things they do want persisted and things they don't want persisted | 11:53 |
| Facu | we can take care of having those fields inside, saving them and loading them | 11:53 |
| niemeyer | Facu: There's no ambiguity when one is using code like self._stored.private_addresss about where that value is coming from, where it goes to | 11:54 |
| niemeyer | Facu: This already works for anything that is an Object in the framework world | 11:54 |
| niemeyer | Facu: So this need is not unknown to us, or something we need a different design for I think | 11:54 |
| Facu | niemeyer, I get that; my point is if we want to encourage people to have "super simple event objects that may carry one or two well-defined pieces of information", or "a generic object with complex machinery that may save some state and flies around" | 11:55 |
| niemeyer | Facu: If we do change it, we need to change everything else with it so we don't have diverging patterns inside the same logic | 11:55 |
| niemeyer | Facu: Yeah, that's what I'm talking about too | 11:55 |
| niemeyer | Facu: I don't see self._stored.private_address as worse than self._private_address + a list of fields elsewhere | 11:56 |
| niemeyer | Facu: The list of fields means you'll need to read the list of fields again every time you want to know which of the 10 attributes are persisted or not | 11:56 |
| niemeyer | Seems very error prone and unfriendly | 11:56 |
| Facu | all those fields are persisted | 11:57 |
| niemeyer | Facu: All the fields in the list of fields are persisted | 11:57 |
| niemeyer | Facu: All the fields not in the list of fields are not persisted.. right? | 11:57 |
| Facu | there are not "fields not in the list", we don't allow them, we enforce the Events to be dead simple | 11:58 |
| Facu | that pastebin I shown holds *all the code* for the Event the user needs to write (it was not just the beginning) | 11:58 |
| niemeyer | Facu: So what happens when one needs any kind of logic in the event? Then the dead simple becomes painful? | 12:01 |
| niemeyer | Facu: Again.. we have a solution for this problem, because it's a common problem. If our solution needs further improvements, let's do so for everything. | 12:01 |
| Facu | we want developers to add logic to the event? if yes, let's go with developers handling what they store by hand, yes | 12:01 |
| niemeyer | Facu: If we want people to be able to use normal Python code in their types? Yes | 12:02 |
| niemeyer | Facu: I hope you can see the value in us defining a strong pattern that solves that kind of issue nicely and consistently | 12:03 |
| niemeyer | Facu: Does StoredState work in events today? | 12:04 |
| Chipaca | niemeyer: standup? | 12:16 |
| niemeyer | 11:42:15 <niemeyer> I'm going to skip the standup today as I'm researching something for a call with Mark in a bit.. I've been reviewing and will be reviewing is my main action points | 12:17 |
| Chipaca | niemeyer: missed that. thanks. | 12:29 |
| Chipaca | niemeyer: good luck with the call | 12:29 |
| niemeyer | np, and thanks | 12:32 |
| jam | Chipaca: no, its just me trying it out. ":return: None" is probably better. | 12:34 |
| Facu | niemeyer, yes, I totally see the value, my other proposal was a way to super restrict users in what they could do with the events, if we wanted that | 12:47 |
| Facu | niemeyer, I would need to explore if StoredState works in events today; without exploring/seeing code I don't envision how we could restore/unfreeze an Event that was deferred and make external code to access what the event has stored | 12:49 |
| niemeyer | Facu: Restoring data is exactly what StoredState does.. both deferred Events and StoredState use the normal persistence system of the framework | 12:51 |
| niemeyer | Things are not so far apart as they may seem | 12:51 |
| niemeyer | It probably already works if we make EventBase a child of Object | 12:51 |
| Facu | let me play with the code a little | 12:52 |
| Facu | this is weird, I just noticed that the timestamps from "juju debug-log" are local (GMT-3), not UTC (which is how the units are configured) | 14:15 |
| Facu | which makes me wonder... when I see | 14:15 |
| Facu | unit-my-super-charm-5: 11:13:56 INFO juju.worker.uniter.relation joined relation "my-super-charm:dumper postgresql:db" | 14:15 |
| Facu | ...that means that the event happened at minute 13 second 56, or that juju is showing me the message at that time? | 14:16 |
| Dmitrii-Sh | https://github.com/canonical/operator/pull/157#discussion_r392289930 | 15:15 |
| niemeyer | Facu: I don't know the response to that.. It would be nice if what you see _locally_ in debug-log is in the local timestamp, though | 15:16 |
| niemeyer | Facu: I believe the data is stored in UTC, because that's what the db driver will do by default | 15:16 |
| niemeyer | Facu: But.. don't trust me :) | 15:16 |
| Facu | so it probably shows me the original timestamp, shifted for my local timezone | 15:17 |
| niemeyer | Dmitrii-Sh: As I said there, we seem to be in very different wavelengths | 15:18 |
| niemeyer | Dmitrii-Sh: I am (and Chipaca is) trying to explain things in a way that makes sense to users | 15:18 |
| niemeyer | Dmitrii-Sh: Saying that we're returning an "app endpoint binding" doesn't make any sense in this context | 15:19 |
| niemeyer | Dmitrii-Sh: The API allows people to bind specific spaces to endpoint names, and to specific relations established | 15:19 |
| niemeyer | Dmitrii-Sh: That's the general framework people will have in mind when they're coding or using the system | 15:19 |
| niemeyer | Dmitrii-Sh: The terminology we pick needs to make sense inside that framework | 15:20 |
| niemeyer | Facu: I *hope* so | 15:20 |
| Facu | niemeyer, good enough | 15:20 |
| Dmitrii-Sh | niemeyer: I think you are right about naming. My suggestion lacks an adjective for "non-relation-specific" bindings. | 15:24 |
| Dmitrii-Sh | You could have two apps related via their relation endpoints bound to different spaces as of today AFAIK. It doesn't make a lot of practical sense in my view but relation instances themselves cannot be bound to spaces. | 15:24 |
| Dmitrii-Sh | i.e. one can't say | 15:25 |
| Dmitrii-Sh | bind "1=myspace" | 15:25 |
| Dmitrii-Sh | for relation_id=1 | 15:25 |
| niemeyer | I don't think I understand what you're trying to say... we accept a relation instance in that method precisely because people can bind spaces to specific relation IDs.. network-get takes an ID parameter doesn't it? | 15:29 |
| Dmitrii-Sh | niemeyer: network-get accepts a -r <id> parameter, however, juju doesn't allow things like `juju bind <relation-id>=<space>` or `juju relate <appA> <appB>` via a space | 15:41 |
| niemeyer | Dmitrii-Sh: Sorry, can we focus on that first part for a moment | 15:41 |
| niemeyer | Dmitrii-Sh: What does that mean that network-get accepts a relation ID? | 15:41 |
| Dmitrii-Sh | One can use `network-get <endpoint-name>` or `network-get -r <rid> <endpoint-name>` https://github.com/juju/juju/blob/juju-2.6.7/worker/uniter/runner/jujuc/network-get.go#L83 | 15:43 |
| Dmitrii-Sh | -r <rid> could only provide a different output in the case of CMR | 15:43 |
| niemeyer | Dmitrii-Sh: If you may get a different binding when you provide an ID, that means a relation instance may in fact be bound to a particular space, right? | 15:44 |
| Dmitrii-Sh | Not at the moment, a relation id only affects the selection of ingress-address and egress-subnets when CMR is used. It's difficult to explain because the logic is convoluted at the Juju side: | 15:47 |
| Dmitrii-Sh | https://github.com/juju/juju/blob/juju-2.7.4/apiserver/facades/agent/uniter/networkinfo.go#L142-L155 | 15:47 |
| Dmitrii-Sh | https://github.com/juju/juju/blob/juju-2.7.4/apiserver/facades/agent/uniter/networkinfo.go#L310-L334 | 15:47 |
| niemeyer | Dmitrii-Sh: Man.. can you please stop sending me links? :) | 15:47 |
| Dmitrii-Sh | OK, simply put: as a user, I cannot specify a space per relation ID - only per endpoint name. | 15:48 |
| niemeyer | Dmitrii-Sh: We're documenting the juju CLI | 15:48 |
| niemeyer | Dmitrii-Sh: We're documenting the operator framework API | 15:49 |
| niemeyer | Dmitrii-Sh: network-get takes a parameter which is a relation ID | 15:49 |
| niemeyer | Dmitrii-Sh: Your code accepts a parameter which is a relation instance.. the method name is get_binding | 15:49 |
| niemeyer | Dmitrii-Sh: This is a very simple story.. the confusion comes if we try to reject what is clear | 15:50 |
| niemeyer | Dmitrii-Sh: We're NOT documenting the juju CLI | 15:50 |
| niemeyer | The alternative here is to change the API so that we don't do any of that | 15:52 |
| niemeyer | Taking details out of the binding information | 15:52 |
| niemeyer | I don't know how to do that cleanly, though, because that's a "network binding", and these are network binding details | 15:53 |
| niemeyer | If we take egress, ingress, etc out of the network binding details, where do we put them? | 15:53 |
| Dmitrii-Sh | not `Relation`s because they might not exist at a given point in time | 15:55 |
| Dmitrii-Sh | As an idea, since apps have endpoints: self.model.app.<something> | 15:59 |
| niemeyer | How about this: specific relation instances may have different network bindings | 16:00 |
| Dmitrii-Sh | so, relation.binding.network.ingress_address would only work if you had a reference to a relation | 16:01 |
| Dmitrii-Sh | but get_binding(relation_name) would give you a relation-less binding | 16:01 |
| niemeyer | Dmitrii-Sh: We might also make get_binding accept a relation instance | 16:01 |
| Dmitrii-Sh | well, then we are back to the naming conversation | 16:02 |
| niemeyer | !!! | 16:03 |
| Dmitrii-Sh | ok, sorry, I didn't get your point: | 16:07 |
| Dmitrii-Sh | 1) relation.binding - OK, we could do that | 16:07 |
| Dmitrii-Sh | 2) get_binding(relation_instance) - OK, it's in the framework already | 16:07 |
| Dmitrii-Sh | what's the catch? | 16:07 |
| niemeyer | The catch is this is a long proposal to change something that apparently works, matches what juju does, and is relatively clean, without a rationale | 16:09 |
| niemeyer | We got into this conversation because I was proposed three lines of documentation to explain what was going on | 16:10 |
| niemeyer | You disagreed.. and I asked: okay, we have these three scenarios in this method.. how do we document them? | 16:11 |
| niemeyer | Which then got you to argue with dozens of links :) about how we don't relation-specific bindings | 16:12 |
| niemeyer | *don't have | 16:13 |
| niemeyer | Except, well, apparently we do otherwise we cannot explain the behavior | 16:13 |
| niemeyer | So.. let me know how you want to document it cleanly :) | 16:13 |
| Dmitrii-Sh | ok, I'll think about an alternative for "default binding for that relation" phrasing because it overlaps with "a default binding for all endpoints of an application". I'll suggest it if I come up to something. | 16:19 |
| niemeyer | Dmitrii-Sh: "default for the relation" and "default for the application" only overlap in the "default" part.. default is a very common term that has an important meaning, and is hard to replace | 16:21 |
| Dmitrii-Sh | I agree, I'd be confused though because "default for the relation" is not documented in the Juju docs. I am not suggesting "default binding" is well documented in Juju either but at least it's mentioned :^) | 16:23 |
| Dmitrii-Sh | Either way, replacing a docstring is easier than replacing an API. | 16:23 |
| niemeyer | @Dmitrii-Sh Which is exactly why I outlined the three points in my comment in the PR | 16:24 |
| niemeyer | Full circle now.. we have bindings for a relation name, bindings for a relation instance that may return the bindings for that instance, and bindings for a relation instance that actually return the bindings assigned to the relation name | 16:24 |
| niemeyer | The third case is a fallback | 16:24 |
| niemeyer | An easy way to explain a fallback is with the "default" word | 16:25 |
| Facu | Ok, I can not use my system's autopep8 because it seem it's too old | 16:47 |
| Facu | I'll use a venv, with system-site-packages so I get PyYAML from the system, which is what we wanted to do | 16:48 |
| Facu | however, now all the test fails because (for example) files inside the env doesn't have a copyright header :p | 16:49 |
| Facu | (I also had to move my test charm out of this directory for the same reason) | 16:49 |
| Facu | Chipaca, any objection that we specify "ops" and "test" in the Makefile commands? otherwise it's too intrusive, not letting anybody have other stuff in the same dir while developing | 16:50 |
| Dmitrii-Sh | niemeyer: ok, I think we can settle on "default" for now | 16:50 |
| Dmitrii-Sh | thanks for guiding me through the thought process behind it | 16:51 |
| Chipaca | Facu: and setup.py i guess :-) | 16:51 |
| Chipaca | Facu: and … anything else? | 16:51 |
| Facu | Chipaca, setup.py already specify packages=["ops"], | 16:52 |
| Chipaca | Facu: i mean the other way around | 16:52 |
| Chipaca | Facu: setup.py should be held to the same standard as the rest of the code | 16:53 |
| Facu | ah | 16:53 |
| Facu | would you mind to explain me the rationale of using *both* autopep8 and flake8? | 16:57 |
| Facu | if we're trying to have our code compliant to pep8, have a flake8 guardian should be enough | 16:58 |
| Chipaca | Facu: i'm not familiar enough with the details of those two to know what they cover | 16:58 |
| Chipaca | or how much overlap they have | 16:58 |
| niemeyer | I can explain.. | 16:59 |
| niemeyer | Facu: We want to stick to flake8 as it's probably the most common tool use for this and it does a good job | 16:59 |
| niemeyer | Facu: We use autopep8 to automatically do that for us | 16:59 |
| Facu | yes, everybody uses it | 16:59 |
| niemeyer | Facu: The goal of using both is to ensure that we remain compatible with flake8, but we want to get the hints that autopep8 give us to tell how to fix it | 17:00 |
| niemeyer | *gives us | 17:00 |
| niemeyer | That's why autopep8 comes first as well | 17:00 |
| niemeyer | And that's why travis runs it with --diff | 17:00 |
| niemeyer | Instead of telling us "You guys suck", it says "You guys suck in those specific ways, and this is how you should not suck" | 17:01 |
| Facu | ack | 17:02 |
| Facu | would you mind if I run these two as part of the unit test infrastructure (together with the grep and find for copyright and backslashes)? | 17:02 |
| Facu | that would allow us to have a more simple indication on where everything must run, and get rid of the Makefile | 17:03 |
| * Facu would leave just a "test" script | 17:03 | |
| niemeyer | We don't need to run both all the time.. I'd run just autopep8 which is the one that gives us more details | 17:04 |
| niemeyer | Keep flake8 in travis as a double check | 17:04 |
| Facu | I can do that | 17:04 |
| niemeyer | Running it in test seems fine, as long as it doesn't take a lot of time to run every time, and that there's a flag to not run it (sometimes you want to test code even if it's messy for the moment) | 17:04 |
| Facu | niemeyer, that's one beneffit of getting it as a unit test, you can have a total messy code, and you will only get one failing test (currently it just blocks the execution of the real tests) | 17:06 |
| niemeyer | I see what you mean, yeah, sounds good | 17:06 |
| Facu | also I'll try also flake8 locally, and see if it's slow or not | 17:06 |
| Facu | A mate, and then the final stretch | 19:36 |
| niemeyer | I'm envy | 19:36 |
| * Facu eods and eows! | 21:12 | |
| Facu | see you all on Monday! | 21:12 |
| niemeyer | Have a good weekend! | 21:12 |
| Facu | niemeyer, same for you! | 21:13 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!