#smooth-operator 2020-04-20
<jam> morning all
<t0mb0> this may or may not be an operator specific problem but i'll ask here - how would one ensure a container is secured with TLS? Should the operator framework create an ingress or is that a Juju thing? jam you may know?
<t0mb0> I would have imagined that the operator pod would form a http relation with the ingress perhaps?
<t0mb0> I've set the charm options juju-external-hostname and kubernetes-ingress-allow-http=true however I'm not seeing any ingresses being created!
<t0mb0> note - I am using microk8s not charmed k8s
<niemeyer> t0mb0: The answer depends on the charm and the interface being used indeed.. TLS is used by several independent protocols (we're doing IRC over TLS just now)
<niemeyer> t0mb0: Is the question specific to microk8s?
<jam> t0mb0, have you done "microk8s enable ingress" ?
<jam> https://microk8s.io/docs/addons
<jam> I believe for Juju itself you need to enable "dns" and "storage" but if the applications you're deploying need other K8s services like an ingress controller, then you need to have those enabled as well.
<niemeyer> Morning jam
<jam> morning niemeyer
<t0mb0> jam, thanks! I believe microk8s enable ingress was what I was looking for
<t0mb0> thanks for your input too niemeyer
<niemeyer> t0mb0: np, glad it's sorted
<jam> hi Chipaca . just in time for me to step out for lunch :)
<Chipaca> jam: good morning! buen provecho :)
<niemeyer> > We should be consistent on ordering. However, I do believe 'unittest.assertEqual' assumes (expected, actual) ordering. Because if you do:
<niemeyer> jam: Added another comment there.. I don't think that's true.
<niemeyer> Let's please keep the usual obtained, expected order.. even the examples in unittest.py itself order like that.
<niemeyer> Also: https://github.com/python/cpython/tree/3.7/Lib/unittest/test
<Chipaca> niemeyer: what should I see in that link?
<niemeyer> Tests? :)
<niemeyer> Chipaca: It's the tests for unittest itself
<Chipaca> ah, gotcha
<niemeyer> Chipaca: All of them follow obtained, expected..
<niemeyer> So it'd be slightly awkward if the author intended the opposite
<Chipaca> i wonder why they chose the diff to go in the opposite direction then
<niemeyer> Chipaca: I don't see it as reversed
<niemeyer> Chipaca: I actually don't even know what I expect myself in that sense, though, to be honest
<Chipaca> fair
<jam> so my first hit for: "python unittest assertEqual expected" was that it was "expected, actual" according to xUnit conventions
<jam> Python itself carefully says "first, second"
<niemeyer> jam: Not so carefully, though.. just read the docstrings of these functions
<jam> but the diff does read backward in my head for that. (this has been added / missing relative to?)
<niemeyer> self.assertEqual(the_exception.error_code, 3)
<niemeyer> This is in the docstring of assert raises
<niemeyer> I find ("foo" == var) inverted given years of reading it the other way around, but I wouldn't mind adapting if we were following a pattern.. But we are not.. we'd both be making it awkward based on common style, and on the intended form of the testing library we use, so I'm -1 on it
<niemeyer> jam: Isn't xunit coming from Java?
<jam> https://bugs.python.org/issue10573 seems to be the bug, mfoord raised the issue of ordering so you get appropriate diffs
<jam> https://mail.python.org/pipermail/python-dev/2010-December/106954.html is Guido's comment on it
<jam> (old, new) matches diff and matches (expected, actual), but intentionally using "first, second" in the code instead.
<jam> so, its fine, we can go with unittest's convention, but it does mean that all the diffs are "how expected differs from actual" rather than "how actual differs from expected"
<niemeyer> This second link makes me sad..
<niemeyer> It's a glimpse of poor leadership..
<niemeyer> "Yeah, we do use that ordering in our code everywhere, but people elsewhere use different conventions.. so we don't care.. do whatever."
<niemeyer> Multiply that by 3 decades, and bingo..
<jam> there is also https://youtrack.jetbrains.com/issue/PY-26471 for the ordering in PyCharm.
<jam> And their example tutorial actually puts it as assertEqual(foo, 10), but then prints Expected: 'foo', Actual: 10.
<jam> First, Second at least tells you where to look and makes you actually figure out which is expected and what is actual.
<jam> I'll clean up Harness.
<jam> to conform
<niemeyer> Haha, priceless :P
<mup_> PR operator#235 opened: test/test_harness.py: Fix the ordering to assume actual/expected <Created by jameinel> <https://github.com/canonical/operator/pull/235>
<facubatista> Muy buenos dÃ­as a todos!
<jam> morning facubatista
<facubatista> hola jam
<niemeyer> Morning Facundo
<facubatista> hola niemeyer :)
<facubatista> jam, I don't understand your comment about not replacing 'juju_exec_path' in Chipaca's branch
<jam> facubatista,         juju_exec_path = juju_exec_path.parent / dispatch_path
<facubatista> jam, IIUC, we need to check if the juju_exec_path exist *and if it's not ourselves* (that check still missing), and only then execute it... but if we decide to execute it, we'll replace it, right?
<jam> If you replace it, it becomes harder to tell "if it is ourself"
<facubatista> jam, but that verification should be done before
<jam> facubatista, well, I would expect to be done right around that point, as that is the point you are seeing if it exists and what it points at
<Chipaca> yes that check is still missing, fwiw
<facubatista> if juju_exec_path.exists() and juju_exec_path.resolve() != Path(sys.argv[0]).resolve():
<facubatista> jam, I'd do something like ^
<jam> facubatista, sure. my point was that juju_exec_path was already Path(sys.argv[0]), so you could just use it if you don't replace the var first.
<facubatista> ack
<pekkari> Hi guys, is there currently a path to retrieve the ip address of a unit? Model seems to hint you have to traverse the BindingMapping of the full model if you want so
<Chipaca> pekkari: I don't think there is another way
<Chipaca> pekkari: i'm not even sure there is an ip address of a unit without reference to a binding, so it's not clear to me this is a deficiency
<jam> pekkari, in the juju model, you might be deployed to a machine that has multiple network cards and the operator wants different applications on different interfaces, so you have to go via the binding so the operator can tell you how that piece is configured.
<jam> there is no "one true IP" for a unit as a whole
<pekkari> yeah, multiple addresses is certainly something I understand, I'm just struggling to find from a charm event the way up to an ip address related to the binding of the relation we run on
<jam> pekkari, def _on_changed(self, event): self.model.get_binding(relation_name).network ?
<jam> are you in a Charm, or in a separate component ?
<jam> binding key can be a relation as well, so if you wanted
<pekkari> jam that may be it
<jam> self.model.get_binding(event.relation)
<pekkari> I'm in a charm, defining the on_identity_service_relation_joined, and there I need the ip of the binding it talks to keystone
<jam> pekkari, I don't think you tell Keystone about the network details to keystone. I would expect you to tell keystone about some other binding.
<pekkari> erm no, this charm tells it's ip address in the binding to keystone to setup the endpoint url
<jam> pekkari, OS traditionally had public/private/admin bindings to describe the application you were deploying, so that would be, self.model.get_binding('public')
<jam> pekkari, I would expect that a sysadmin would configure the binding of the keystone endpoint to "how keystone should talk to my app" but what you want to tell Keystone is "how should other apps (eg neutron/nova/etc) talk to my app"
<jam> which is not the binding of the relationship to keystone, but a different binding on your charm.
<pekkari> that is very true, however hardcoding the public binding makes me wonder if there is a better way to put it
<pekkari> certainly may not be a problem, at the end of the day, you only want the service to be reached in public space
<jam> pekkari, if its your charm, you define what endpoint you are advertising to keystone
<pekkari> it may be right jam, as long as if you want another you can simply configure the public binding to point to another space
<jam> if it is "all openstack charms", then you can pass in the name of the public binding in __init__ and have your code track that.
<jam> pekkari, correct. that is the point of having an endpoint that sysadmins can bind
<pekkari> thanks for the guidance!
 * facubatista brb
<mup_> PR operator#235 closed: test/test_harness.py: Fix the ordering to assume actual/expected <Created by jameinel> <Merged by jameinel> <https://github.com/canonical/operator/pull/235>
<Chipaca> facubatista: didn't #209 fix #198 ?
<mup_> PR #209: Enable stderr logging if JUJU_DEBUG is set <Created by jameinel> <Merged by jameinel> <https://github.com/canonical/operator/pull/209>
<mup_> Issue #198: DEBUG log messages not emitted <bug> <Created by stub42> <https://github.com/canonical/operator/issues/198>
<Chipaca> jam: or you :)
<jam> Chipaca, no. I didn't do both at the same time. We still only put INFO messages to juju-log, as it wasn't clear to me if we wanted to change that behavior. I wasn't clear why we didn't default to DEBUG from the start, so I needed to understand that first.
<jam> its a trivial change to change the level of that logger
<jam> Chipaca, https://paste.ubuntu.com/p/3xf4gPctxV/
<jam> its just a question of why was it at INFO to start with, and if we chose to change it, is a small fix for 198
<facubatista> jam, Chipaca, we want that by default only INFO messages go to juju, and if charm developer decides to send DEBUG too, to be achievable (users always can control the level they see through `juju debug-log`, anyway)
<facubatista> for that behaviour to happen, we need:
<facubatista> - the juju handler to be in DEBUG level
<facubatista> - the logger to be in INFO by default <-- this is what the charm developer can control
<facubatista> I still didn't prepare a branch for #198 because I still do not understand how the charm code could change the log level according to something external, and if we could do that automatically
<mup_> Issue #198: DEBUG log messages not emitted <bug> <Created by stub42> <https://github.com/canonical/operator/issues/198>
<facubatista> (so the charm would emit INFO or DEBUG according to something, not harcoded)
<jam> facubatista, so if we call 'juju-log' then Juju itself will filter based on 'juju model-config logging-config'
<jam> facubatista, we could ask for a way for juju to tell us a given unit's logging config, if we wanted to avoid triggering juju-log in the first place
<facubatista> jam, but we'd need to ask that for every message, right? as it may change at anytime
<jam> facubatista, so it can, but I could say that we don't notice a change inside a single hook execution
<facubatista> jam, I like that optimization! JujuLogHandler will stop executing an external process for each message if it makes no sense
<facubatista> jam, and all we need to do for that is set the level in JujuLogHandler according to what the config of the unit
<jam> facubatista, it would need a juju change, so we'd have to deal with versions-of-juju, etc.
<facubatista> if we have it, we use it
<Chipaca> facubatista: jam: i think it's an optimisation we can add when/if juju gets the feature
<facubatista> it's a good way also to "control the level of the charm"?
<facubatista> or there's a better way?
<facubatista> my point is:
<facubatista> I have 5 units with 5 charms; I call `juju debug-log` with level in DEBUG because I want to see debug messages of *one* of those charms, the rest I want to see them in INFO ... which is the best way to achieve that?
<facubatista> jam, which is the unit's logging level by default?
<Chipaca> facubatista: juju debug-log --include=theunit ?
<Chipaca> The '--include' and '--exclude' options filter by entity. The entity can be
<Chipaca> a machine, unit, or application for vm models, but can be application only
<Chipaca> for k8s models.
<facubatista> Chipaca, that will not show me the other ones ... my point is seeing all of them, one in DEBUG the rest in INFO
<facubatista> jam, how do I change the unit's logging config?
<jam> facubatista, 'juju model-config logging-config="unit.?=INFO"'
<jam> by default 'unit=DEBUG'
<jam> you can do
<jam> 'juju model-config logging-config'
<jam> to see what the default ais
<facubatista> <root>=INFO;unit=DEBUG
<facubatista> root?
<jam> juju model-config "logging-config=<root>=INFO;unit.unit-postgresql-0=DEBUG;unit=INFO"
<jam> facubatista, ^^
<Chipaca> parallalysed == paralysed by parallelism
<jam> niemeyer, https://github.com/canonical/operator/pull/196 still needs your follow up review
<mup_> PR #196: Factor out the Harness changes for test_model.py <Created by jameinel> <https://github.com/canonical/operator/pull/196>
<jam> facubatista, I think #230 doesn't have a comment from you yet. but I think I addressed Chipaca's comments.
<mup_> PR #230: Testing harness no events for own data <Created by jameinel> <https://github.com/canonical/operator/pull/230>
<jam> niemeyer, looking at 196, I see a couple things that I think have been superseded. wait on that one until tomorrow.
<niemeyer> jam: ack
<jam> Id like to land 230 and then clean up the diff
<facubatista> jam, will check
<jam> th
<jam> thx
<Chipaca> travis is slow today
<Chipaca> i guess it's because we share org with something else
<niemeyer> jam: Added a comment in #230
<mup_> PR #230: Testing harness no events for own data <Created by jameinel> <https://github.com/canonical/operator/pull/230>
<niemeyer> jam: As I said, looks reasonable overall.. was just curious about that small detail
<Chipaca> how reasonable would it be to make it so that, if you execute your charm directly with no JUJU_ stuff defined (or maybe the trigger is running it from a terminal) you get an interactive session?
<Chipaca> like ipython with the charm already built for you to play with
<jam> Chipaca, so there are a bunch of bits that you'd need (what is your unit name, what app are you, what is your config, etc). that makes it a bit hard to be initialized and interactive  without sufficient context to know what to actually do
<jam> what 'hook' are you wanting to run right now.
<niemeyer> Chipaca: That sounds like a pretty cool idea
<niemeyer> jam: Imagine dropping it in a shell with a Harness
<facubatista> that would be neat
<Chipaca> if we could do it without a harness even better (but yeah that'd require _some_ JUJU_ stuff
<Chipaca> um
<jam> niemeyer, I think having a good way to do that is great. I don't think 'dispatch' would quite be the entry point for that.
<Chipaca> does 'juju ssh' give you JUJU_ things?
<jam> Chipaca, 'juju run --unit X' does, IIRC
<niemeyer> jam: It might error out nicely saying "Hey, you are not in a juju environment.. if you wanna play, try dispatch --foobar"
<Chipaca> yeah something like that
<Chipaca> all the things :)
<niemeyer> Chipaca: Not sure we could do without a harness.. as jam mentioned, there's a bunch of things required. It's hard to do something when there's no data, no CLIs, no ...
<Chipaca> yep
<Chipaca> i'll try to spec it out a bit more so we can discuss (mostly just with examples of what i'd like to see)
<Chipaca> now that i know it's not completely zany :)
<niemeyer> Thanks for that
<jam> niemeyer, so 'relation.data[entity]' doesn't load that entities data until you do something with it. It actually doesn't even load the data if you do "relation.data[entity]['foo'] = 'bar'" because it doesn't need to know the old content of foo to set it to bar
<jam> it only loads it if you access foo, or iterate or len(relation.data[entity])
<niemeyer> jam: Makes sense. Would you mind to add a short note stating that, just to remind the next reader?
<jam> sure
<niemeyer> Thanks!
<jam> niemeyer, I confirmed with some debug statements inside the _load when you wouldn't want it to be loaded. :)
 * jam heads for EOD
<niemeyer> Cheers
<niemeyer> jam: Have a good EOD
 * Chipaca shakes fist at multipass
 * facubatista -> lunch
<Chipaca> stub: ð
<Chipaca> stub: is there a usable-for-simple-stuff postgres interface I could use?
<Chipaca> facubatista: 2FA shenanigans, i'll be there as soon as it lets me
<Chipaca> wait, google signed me out everywhere again
<Chipaca> sigh
<Chipaca> facubatista: I'm guessing something's come up at your end as well. Let's reschedule.
<mup_> Issue operator#231 closed: Harness: Has errors on charms which define action handlers <Created by Vultaire> <Closed by chipaca> <https://github.com/canonical/operator/issues/231>
<mup_> PR operator#232 closed: ops.testing.Harness actions metadata support.  (#231) <Created by Vultaire> <Merged by chipaca> <https://github.com/canonical/operator/pull/232>
<niemeyer> Chipaca: Is there something holding #203 back still?
<mup_> PR #203: ops, test: revert the EventSet change, make EventsBase ObjectEvents <Created by chipaca> <https://github.com/canonical/operator/pull/203>
<Chipaca> niemeyer: a review? (and now a deconflict also)
<niemeyer> We should make a push to these old PRs through or out
<niemeyer> Chipaca: My +1 has 12 days
<facubatista> Chipaca, grrr, had the alarm off, sorry
 * facubatista is back
<Chipaca> facubatista: huh
<Chipaca> facubatista: https://github.com/canonical/operator/blob/master/test/test_framework.py#L773
<Chipaca> is a bit weird
<Chipaca> I think you meant FooEvent(EventBase)
<Chipaca> I'm mostly-EOD. Might pop in later to tinker with dispatch a little as i'm behind on where i wanted to be.
<Chipaca> ð
 * facubatista brb
<facubatista> niemeyer, did you read this? https://setuptools.readthedocs.io/en/latest/pkg_resources.html#entry-points
<niemeyer> facubatista: I hadn't, will look in detail tomorrow, thanks!
<facubatista> :)
 * facubatista eods
<mup_> PR operator#203 closed: ops, test: revert the EventSet change, make EventsBase ObjectEvents <Created by chipaca> <Merged by chipaca> <https://github.com/canonical/operator/pull/203>
#smooth-operator 2020-04-21
<jam> morning all
<niemeyer> Morning jam
<niemeyer> Morning all
<jam> morning niemeyer
<mup_> PR operator#236 opened: ops/log.py: Default to DEBUG logging <Created by jameinel> <https://github.com/canonical/operator/pull/236>
<niemeyer> jam: Reviewed, thanks
<jam> thanks for the review
 * jam goes to make lunch
<Chipaca> gooooood morning!
<niemeyer> Hey hey
<stub> Chipaca: https://code.launchpad.net/~stub/interface-pgsql/+git/operator , which needs a better repo home and updating to namespace standards when the standards exist :)
<niemeyer> stub: I hope to have this settled before the week is over
<stub> Yup, no hurry from my end
<niemeyer> stub: We also need to have some kind of group for reviewing interfaces while we try to develop some joint agreement of which directions we want to go
<niemeyer> stub: This sounds like the right time for that
<niemeyer> stub: That is, now that we're about to have the basic proposal in place for the extension mechanism and the namespacing
<niemeyer> stub: Would be happy to have you on board this effort
<stub> ok, let me know if I'm needed when you get that far.
<niemeyer> stub: Your presence would certainly be good, given your prior experience in the postgres charm and interfaces.. we also need to do a review of these interfaces to evolve our idea of which way we want to go as recommended patterns
<niemeyer> stub: Did you inspire the initial implementation on the sketches that were being discussed here, or completely independently?
<stub> (and that intereface needs tests, so probably would fail review right now ;) )
<niemeyer> stub: Well, that's another area worth talking about.. we've been evolving the Harness, so it would be interesting to know how suitable it is for working with interfaces as well
<stub> niemeyer: Somewhat independently... based on some examples (graphana request for comments I recall)
<niemeyer> stub: Cool, let's please try to find a slot that works for everyone over the next few days and get that going
<niemeyer> stub: I will have the mechanism and namespace proposal in hand by then
<Chipaca> stub: thanks!
<Chipaca> graaah
 * Chipaca hulk-smashes
<Chipaca> why would _simulate_event itself clear the charm state
<Chipaca> that should be done by tear-down
<Chipaca> stoopid stoopid
<niemeyer> Me too, but in a good way.. manual k8s deployment seems to work okay
<Chipaca> and of course the test charm doesn't load the state before writing it out
<Chipaca> it's all worms all the way down
<Chipaca> in fact why is this doing it by hand
<Chipaca> â¦ D'OH
<jam> Chipaca, can you point me at what you're talking about? _simulate_event is not something I'd generally recommend.
<Chipaca> truncate() and truncate(0) are *not* the same thing
<Chipaca> jam: hi :)
<Chipaca> jam: it's in test_main.py
<Chipaca> jam: there's a combination of issues that made it hard to figure out for me
<Chipaca> jam: the test_main charm nukes its state every time
<Chipaca> ie it does not load state, just sets it to default
<Chipaca> not helped by doing things by hand instead of using StoredState
<Chipaca> then, _read_and_clear state does truncate() instead of truncate(0)
<Chipaca> so i fixed one side of that and then was confused by the other side breaking
<jam> Chipaca, in each test? I would expect that, otherwise the tests would be coupled and you couldn't just run 1 test. But _simulate_event doesn't, does it?
<jam> Chipaca, oh.
<Chipaca> jam: in test_main/src/charm.py, in __init__, it just does _state = {}
<Chipaca> so no matter how many times dispatch dispatches an event you'll only get the one event in the state
<jam> load and then truncate, but don't actually do anything because you just read a bunch of stuff so truncate() is a noop, right?
<Chipaca> nah, that bit is fine (in a twisted kind of way), it truncates it so the _next_ call gets the data ok
<Chipaca> i wouldn't write it that way but it works (assuming truncate(0) and not truncate() as it was written)
<jam> Chipaca, that's what I mean. Plain truncate() is a no-op
<Chipaca> ah, yes
<jam> you just read the filefor pickle.load()
<Chipaca> jam: i'm not sure we're in sync, so let me replay events for you:  i was trying to write a test that made sure that dispatch + a hook that pointed to the same thing only triggered the event once
<Chipaca> jam: i wrote the test, and it passed! but that made no sense, and adding prints to the charm code showed me it was processing the events twice
<Chipaca> jam: then i noticed that __init__ was not loading the state, just setting it every time, so that's why it was failing
<Chipaca> or rather, failing to fail
<Chipaca> jam: so i fixed that
<Chipaca> jam: and then other tests started failing!
<Chipaca> jam: because truncate() instead of truncate(0)
<Chipaca> jam: <the end>
<Chipaca> i think it needs refactoring to used StoredState at some point, but for now this seems ok to me
<Chipaca> not ideal :) but ok
<jam> Chipaca, yeah, so we only ever write the state file, we wb which means we always write from the beginning, and we never read it to include more events...
<jam> Chipaca, so, between updating the tests to not overwrite the state file on each call, and using StoredState, which do you think you prefer?
<jam> Chipaca, I feel like StoredState would be more the way we want the charm written, and means it would get auto loaded and saved rather than explicitly done by the charm
<Chipaca> jam: I think we want to rewrite it to use StoredState because people will pick up on this way of doing things if not
<jam> indeed
<Chipaca> jam: but I think it needs doing in a separate PR as it'll be a big refactor
<jam> looking at it right now
<Chipaca> jam: ideally _after_ i land this which already touches test_main :-3
<jam> Chipaca, so in setUp we end up calling 'exec_module'. Does that exec the module with __name__ == __main__ ? or is it just effectively 'import the module into a namespace' ?
<facubatista> Muy buenos dÃ­as a todos!
<jam> morning facubatista
<jam> any chance you could look at #230 today?
<mup_> PR #230: Testing harness no events for own data <Created by jameinel> <https://github.com/canonical/operator/pull/230>
<facubatista> hola jam
<facubatista> jam, yes!
<jam> great
<Chipaca> jam: it does not exec the module with __name__ == __main__, no
<jam> Chipaca, k. its essentially just 'import' but return the module as an imported thing, rather than into sys
<Chipaca> jam: it execs it with __name__ being the "fully-qualified name of the module"
<Chipaca> jam: the import has two parts, loading the code object, and executing it
<Chipaca> bah, loading also has two parts, finding it first :)
<Chipaca> jam: but, yes
<Chipaca> it's 'exec' in the exec-the-keyword sense, not in the exec-the-syscall sense
<Chipaca> jam: note the module is already 'loaded' before the exec call, in that self.charm_module is already <module 'charm' from '<...>/test_main/src/charm.py'> before you exec it
<jam> Chipaca, presumably it doesn't have any types, etc defined, right?
<Chipaca> jam: correct, just a name, a source, a loader, etc
 * Chipaca looks
<Chipaca> ['__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__']
<Chipaca> that's all :)
<jam> Chipaca, not critical to my understanding of what is going on. Mostly it is a way to do 'import foo' but not actually pollute your local namespace.
<Chipaca> yep
<Chipaca> also it doesn't touch sys.modules
<Chipaca> not sure why we're not just doing __import__ though Â¯\_(ã)_/Â¯
<Chipaca> (but i haven't looked in detail; there might be some tweak or subtlety i'm not seeing)
<Chipaca> ouch, i must go get lunch otherwise it's all meetings all the time and i'll be eaten alive by feral teenagers
<jam> Chipaca, So I have a different test that just does 'import' but means i clean out sys.path afterward. I think this is 'think about that other charm thing'
<jam> Chipaca, eat hearty
<facubatista> jam, why RelationDataContent is a lazymapping? and not grab all the info at once?
<jam> facubatista, so I didn't write it, but it reads all the data when you access one attribute. it doesn't read keys individually
<jam> it just doesn't read the data if you only write 1 key
<facubatista> jam, do you know why?
<facubatista> it's an optimization?
<jam> facubatista, yeah, I presume it is just to avoid calling relation-get just because you have a relation, but you don't access any data
<jam> and the LazyMapping means that once you *do* read something, you don't pay an overhead for every access, it gets once and caches the content
<jam> for the lifetime of a hook, the content you read isn't allowed to change, so it is fine to cache.
<jam> in Testing, though, we don't want to destroy the whole model and build it back up between steps
<jam> so we _invalidate the parts we know we're changing.
<facubatista> jam, _invalidate() does not "discards current cache content", but "prevents further caching"?
<jam> facubatista, no, it sets _lazy_data to None, so it will be reloaded on next access
<facubatista> so it discards current cache?
<jam> correct
<facubatista> so, let me recap
<jam> LazyMapping has a _data attribute that returns _lazy_data if not None, else load _lazy_data
<facubatista> I just do rel_data = relation.data.get(entity) ... at this moment, rel_data is "empty", the real info is not loaded yet, so it's not cached, right?
<facubatista> it's _lazy_data is still None
<jam> facubatista, if that is all you do, yes. If someone has accessed that object before, the data might already be loaded
<jam> which is what why we want to _invalidate so we know the new content will be loaded.
<facubatista> ok
<facubatista> thanks, I understand now
<jam> Chipaca, so... I have a rough cut of replacing the STATE_FILE with StoredState. But it turns out we're doing some "naughty" stuff.
<jam> Namely, it saves the type(event) for all the events, which then get pickled down to the state file, and are assumed to be reloaded correctly
<jam> and then the test dose: "assertEqual(state.events, [EventType])"
<jam> where EventType is "from framework import EventType", and state.events was unpickled from the charm state file.
<jam> Why are they doing AllHands in the middle of Vienna Product roadmap ...
 * facubatista misses real all hands
<jam> :)
<jam> Yeah. Though to be fair, All-Engineering is about 200 people, which is almost the size of the largest all-hands
<facubatista> jam, yeap, but we don't mix to the rest of the company
<facubatista> jam, it's not about size, but (content) diversity
<facubatista> jam, I added a couple of small details inline in the #230 review; those are probably so small that we may be done with this today
<mup_> PR #230: Testing harness no events for own data <Created by jameinel> <https://github.com/canonical/operator/pull/230>
<jam> facubatista,             # rel_data may have loaded now-stale data, so _invalidate() it. Note that just
<jam>             # accessing rel_data for an entity doesn't load it unless you access an attribute.
<jam> ?
<jam> The point is to note that the act of wanting to invalidate won't itself read the data, but I understand wanting both comments.
<facubatista> jam, but we don't care if it read the data, as we're invalidating it! that is confusing
<facubatista> it looks like "oh, so let's not call to invalidate, and for sure will not be reading the data"
<jam> facubatista, so the concern that G had, was that if we access rel_data, it looks like we read it, just to invalidate it.
<jam> so I'm trying to find a way to express "make sure if it was read, we invalidate it, but we don't force a read here"
<facubatista> the point of that invalidate call is to clear *possible previously* loaded cache
<facubatista> jam, the "Note that just accessing rel_data for an entity doesn't load it unless you access an attribute." is a good piece of information, maybe confuses to be just around the invalidate() call
<jam> facubatista, how about:
<jam> rel_data may be caching now-stale data, so _invalidate() it.
<jam> Note, this won't cause the data to be loaded if it wasn't already.
<facubatista> jam, I like it, may you replace "be caching" with "have cached"?
<niemeyer> Hey folks, can we have the standup a bit earlier today?  I have an errand that I need to run this afternoon and it would be helpful if I could do it a bit earlier
<jam> sure
<jam> facubatista, ^^
<jam> niemeyer, we can, though I have a meeting just before it. but it is lower priority
<facubatista> niemeyer, otp, what about in 10'?
<niemeyer> jam: No worries then.. it's not critical on my end either
<niemeyer> facubatista: Let's do at the usual time
<facubatista> ack
 * Chipaca suddenly realises the time
<facubatista> Chipaca, you mean, in an einstanian way?
<jam> Time flies like an arrow. Fruit flies like a banana
<jam> facubatista, can you check if 230 now matches your expectation?
<facubatista> jam, yes
<pekkari> Hi, qq, is juju triggering any config-changed after upgrade charm?
<Chipaca> pekkari: AFAIK yes, why?
<Chipaca> jam: ^
<pekkari> thanks Chipaca, just to know, the charm I'm working on was having config rewrites in the upgrade path that I believe are redundant, so if config change comes after, I can just drop that code
<Chipaca> pekkari: perhaps relatedly, if the unit is in error state and you force-upgrade the charm, the upgrade-charm isn't triggered but the config-change is (i only know this from notes in our codebase)
<pekkari> seems to be a case I wasn't into it yet, as I was just ensuring I can, from a working unit, upgrade the charm, and get to a ocnsistent state, but good to know
<jam> pekkari, Chipaca : afaict, on the agent side, it doesn't explicitly say "an upgrade was done, thus I must also do a config changed". However, I think if you do an upgrade, the act of requesting an upgrade might create a new config state, since old version of the charm and new version of the charm may have different fields available.
<pekkari> that makes sense, jam, thanks!
<jam> pekkari, a quick test with 'juju debug-hooks' says that we do, indeed, trigger a config-changed with upgrade even if you didn't pass '--config' to upgrade-charm.
<pekkari> awesome, wiping unneeded code then :)
<facubatista> jam, I see that you answered two conversations, but other two are unanswered, and no commit pushed... maybe I'm looking at it wrongly?
<Chipaca> jam, niemeyer, if you have a hook and dispatch, and the first one fails, should the second one still run?
<facubatista> Chipaca, I'd follow the same reasoning of "if you have two registered callbacks for one event, and the first callback fails, should the second still run?"
<Chipaca> facubatista: yeah, and afaict the answer is no
<facubatista> Chipaca, I concur
<Chipaca> github seems to be having issues
<Chipaca> can't comment on a pr right now
<Chipaca> i think that's github's way of telling me to take a braek
 * Chipaca obeys
<facubatista> jajaj
<facubatista> Chipaca, that, or they decided to turn it off forever
<jam> facubatista, so I tried to push, but I think Chipaca had auto-committed to my branch, so it rejected my push. sorry about that. I didn't see the "rejected" on the push
<jam> facubatista, I just confirmed the PR has been updated with my commits. can you look again ?
<jam> Chipaca, I agree, whatever fails first stops the second from executing.
<Chipaca> of course now there are _three_ comments with the same content
<Chipaca> ah well
<Chipaca> at least i'm consistent :-D
<Chipaca> and travis <-> github is now discombobulated
<Chipaca> it's fun all the way down
<Chipaca> i shoulda just carried on running :-)
 * Chipaca is sure he used to know english
<facubatista> "discombobulated"
<Chipaca> facubatista: https://en.wiktionary.org/wiki/discombobulated
<facubatista> nice
 * facubatista eods
<Chipaca> facubatista: ð have a good one
<facubatista> Chipaca, good night!
 * facubatista really goes
 * Chipaca EODs
#smooth-operator 2020-04-22
<jam> morning all
<jam> hm. mup didn't seem to notice the pr I landed and the other pr I closed.
<mup_> Issue operator#237 opened: Feature request: helper function/flag to run initial hooks as juju would execute? <Created by Vultaire> <https://github.com/canonical/operator/issues/237>
<mup_> PR operator#212 closed: Harness: do not emit extra relation changed events <Created by dshcherb> <Closed by jameinel> <https://github.com/canonical/operator/pull/212>
<mup_> PR operator#230 closed: Testing harness no events for own data <Created by jameinel> <Merged by jameinel> <https://github.com/canonical/operator/pull/230>
<niemeyer> Mornings!
<niemeyer> jam: It did, it was just sleeping.. I haven't yet finished the transition that takes it out of my laptop and replaces the global mup
<jam> morning niemeyer
<Chipaca> ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½  ï½ï½ï½ï½ï½ï½ï½ï¼
<niemeyer> \o/
<Chipaca> I fixed my 'beuno' script to also support not greeting anyone in particular :-D
<jam> Chipaca, my IRC client doesn't understand wide characters, but other apps accept copy & paste :)
<Chipaca> jam: heh. strange irc client.
<jam> Chipaca, reviewing Dispatch now
<Chipaca> jam: ta muchly
<Chipaca> jam: out of curiosity, what client do you use?
<jam> HexChat
<niemeyer> That's why.. you need a client that understand more than hex..
<niemeyer> understands
 * niemeyer wastes brain cycles thinking of a conversation involving only a-f
<jam> it is hard to read everything in hex, but eventually you stop seeing the code
<jam> dead beef cafe
<niemeyer> eeaaaa
<Chipaca> jam: I used hexchat as well, and see those characters no problem
<Chipaca> use*
<jam> I think it is probably the font, not sure, though.
<Chipaca> jam: i can also see all of ðð¨ð¥ð, ðð¸ðð¾ðð, ð«ð¸ðµð­ ð¼ð¬ð»ð²ð¹ð½, ðð ð¦ððð-ð¤ð¥ð£ð¦ðð, ð£ð¯ðð¨ð±ð²ð¯, ðððð ððððððð, ðð¡ðððð, ðððð ðððððð, ê±á´á´ÊÊ-á´á´á´ê±, ððððððððð, ððºðð-ðð¾ððð¿, ðð®ð»ð-ðð²ð¿
<Chipaca> ð¶ð³ ð¯ð¼ð¹ð±, ð¨ðð£ð¨-ð¨ðð§ðð ðð¤ð¡ð ðð©ðð¡ðð, ð´ð¢ð¯ð´-ð´ð¦ð³ðªð§ ðªðµð¢ð­ðªð¤, and you already know about ï½ï½ï½ï½ï½ï½ï½ï½ï½.
<jam> Chipaca, now you're just showing off, and while I'm trying to review your pr, too. dangerous :)
 * Chipaca did not do the colourised version
<jam> oddly, I can see "mall-cap" but not the S :)
<Chipaca> jam: in your defense, all these things confuse hexchat to some degree
<Chipaca> just like emoji do
<Chipaca> trying to select them in hexchat for copying is a fun exercise
<Chipaca> (because, unciode widths is a hard problem, as i have mentioned elsewhere)
<Chipaca> also speling unciode
<jam> Chipaca, some things are harder than others :)
<Chipaca> the output of virtualenv -h, at least at version 20, is horrendous
<Chipaca> let's do better than that
 * Chipaca sets the bar low, but it's not an invitation to limbo
<jam> niemeyer, I think https://github.com/canonical/operator/pull/196 is ready for you to look at again, I pulled in the changes from 230
<mup_> PR #196: Factor out the Harness changes for test_model.py <Created by jameinel> <https://github.com/canonical/operator/pull/196>
<jam> Chipaca, a couple of tweaks but overall dispatch lgtm
<jam> Chipaca, any chance you can look at https://github.com/canonical/operator/pull/236
<mup_> PR #236: ops/log.py: Default to DEBUG logging <Created by jameinel> <https://github.com/canonical/operator/pull/236>
<jam> it needs a second +1
<Chipaca> jam: on it
<Chipaca> sorry for the lag, i was going through my notes trying to find the name of the pypi guy
<Chipaca> s/guy/person/
<jam> niemeyer, is it worth having our 1:1? Or should we cancel it for now and just do our regular standup, etc
 * jam goes to make coffee, bbiab
<niemeyer> jam: In a call with Tim, but I had something to catch up with you on.. let's try to have it
<jam> ok
<jam> let me know when you're done
<niemeyer> jam: Will do
<niemeyer> jam: I'm off..
<niemeyer> jam: ... and I'm on
<jam> Chipaca, "diversity of notations around args" you're talking about docstrings?
<jam> Any chance we can actually have a session on that and actually go over the various proposals that you've tried and do something concrete?
<Chipaca> jam: yeah, docstrings. I've got the exploratory work parked, need to get back to it, possibly right after vienna
<Chipaca> jam: just to say, i ain't forgetting :)
<jam> Chipaca, it feels like it would only take 30min or so for us to get a consensus that we could at least write for our next code
<mup_> Issue operator#198 closed: DEBUG log messages not emitted <bug> <Created by stub42> <Closed by jameinel> <https://github.com/canonical/operator/issues/198>
<mup_> PR operator#236 closed: ops/log.py: Default to DEBUG logging <Created by jameinel> <Merged by jameinel> <https://github.com/canonical/operator/pull/236>
<jam> one thing I really don't prefer about the squash-and-close pattern. It leaves all of my local git branches as "this has not been merged into master"
<jam> even if I "Delete Branch" via the github UX, it doesn't delete them from my disk, or my laptop, etc.
<jam> And 'git branch -d' rightly says "don't delete things that aren't merged" unless you really mean it.
<Chipaca> jam: yep
<Chipaca> agreed on that
<Chipaca> also on the how long it'll take us to agree
<jam> Chipaca, I agree that we should agree
<jam> Chipaca, https://github.com/huntdatacenter/ubuntu-lite/pull/1/files adding a test suite to my 'minimal ubuntu' charm. for huntdatacenter
<mup_> PR huntdatacenter/ubuntu-lite#1: add testing setup <Created by matuskosut> <Merged by matuskosut> <https://github.com/huntdatacenter/ubuntu-lite/pull/1>
<Chipaca> jam: what's huntdatacenter?
<jam> https://www.ntnu.edu/mh/huntcloud
<jam> They are a site that is using Juju to drive their Openstack
<Chipaca> jam: ah, scientific cloud thing?
<jam> They have https://jujuna.readthedocs.io/en/latest/
<jam> as a tool for doing CI
<Chipaca> jam: went to the frankfurt sprint?
<jam> Chipaca, yeah, I think so.
 * Chipaca sings jujuna matata
<jam> for the rest of my days
<jam> Chipaca, they have a form of functional testing, trying to figure out if it is worth pursuing further.
 * Chipaca nods
<Chipaca> i'm off to make lunch, bbiab
<facubatista> Muy buenos dÃ­as a todos!
<Chipaca> facubatista: ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½ï½  ï½ï½ï½ï½ï½ï½ï½ï¼
<facubatista> Chipaca, hola! :) I do see wide chars in my hexchat (however, not emoji)
<Chipaca> oð©¬
<Chipaca> sigh
<jam> Chipaca, facubatista coming to daily for bug revue ?
<Chipaca> yep yep
<facubatista> ah, that one? I was about to ask where
<Chipaca> just fighting with google
 * facubatista in login dance
<mup_> Issue operator#171 closed: Support for charm class selector <Created by gnuoy> <Closed by niemeyer> <https://github.com/canonical/operator/issues/171>
<Chipaca> alright, i'm going to step away for a while, go for a run, and come back for my afternoon/evening meetings
<Chipaca> (got one rather late in the day today)
<niemeyer> Chipaca: Have fun there
<Chipaca> niemeyer: :) yep
 * facubatista brb
 * facubatista is back
<Chipaca> hmmmm
<niemeyer> Agreed
<niemeyer> Oh my.. I thought the charmhub meeting was now
<niemeyer> It was half an hour ago..
<Chipaca> facubatista: what's the name of the thing you're charming?
<facubatista> mmm
<facubatista> Chipaca, what do you mean with "name"? :p
<facubatista> Chipaca, it's my blog, something that uses Nikola and isso as main tooling, the blog itself is called "BitÃ¡cora de Vuelo", here:blog.taniquetil.com.ar/
<Chipaca> facubatista: "we're each working on example charms: jam:jaeger  chipaca:tt-rss  facubatista:<>
<Chipaca> "
<Chipaca> <spaceship>
 * Chipaca goes off to make dinner
 * facubatista eods
#smooth-operator 2020-04-23
<mup_> PR operator#238 opened: ops/model.py: Unit.set_workload_version <Created by jameinel> <https://github.com/canonical/operator/pull/238>
<mup_> Issue operator#211 closed: Test harness: a unit receives events about updates to its own unit data bag <bug> <Created by dshcherb> <Closed by jameinel> <https://github.com/canonical/operator/issues/211>
<jam> morning all
<mup_> Issue operator#239 opened: Action writing requires image dependencies <Created by natalytvinova> <https://github.com/canonical/operator/issues/239>
<niemeyer> Morning jam
<niemeyer> Hello all
<jam> morning
<Chipaca> moin moin
<niemeyer> Hey Chipaca
<jam> niemeyer, #196 still needs your ack, I believe both Chipaca and facubatista are happy with it
<mup_> PR #196: Factor out the Harness changes for test_model.py <Created by jameinel> <https://github.com/canonical/operator/pull/196>
<jam> Chipaca, #236 is a small PR that hopefully isn't controversial, I needed to do *some* coding today. :)
<mup_> PR #236: ops/log.py: Default to DEBUG logging <Created by jameinel> <Merged by jameinel> <https://github.com/canonical/operator/pull/236>
<jam> well, helps if I use the right PR: #238
<mup_> PR #238: ops/model.py: Unit.set_workload_version <Created by jameinel> <https://github.com/canonical/operator/pull/238>
<Chipaca> jam: huh, you can set something, but you can't get it?
<Chipaca> that's super weird
<jam> Chipaca, agreed, though it has been that way for about 5 years now, and it hasn't actually been raised as a bug by someone writing a charm
<jam> Chipaca, as a way to tell the admin what is being run, the source of the value in that field is reading something else on the host machine, you might want to read it to see if it changed, but then again, you're just going to set it to the new value.
<jam> which is as expensive as reading it
<facubatista> Muy buenos dÃ­as a todos!
<jam> morning facubatista
<facubatista> hola jam :)
<Chipaca> facubatista: moin moin
<facubatista> hola Chipaca
<facubatista> Chipaca, I was wondering (and reading a little) if we can use GitHub Actions to store the latest commit (partial) hash in a file to be used as a reference in the version string, for "nightlies" or if people runs the project directly from repo
<Chipaca> facubatista: what i've seen people do is have a distinctive "you're running this from an unreleased thing" version
<facubatista> knowing which specific version is a plus
<jam> facubatista, its pretty impossible to get the hash of the thing and commit it, because committing changes the hash..
<jam> it is usually a 'build' step (in a Makefile :) that generates the version.py from 'git describe --dirty'
<facubatista> there's a [push] event that will trigger actions; the problem is that if we commit current hash on pushes, and then we rebase on merge to master, that previous commit (which is stored) is lost in the sense that we will not find it in the logs
<facubatista> if we wouldn't rebase, that could be useful
<jam> facubatista, you'll always be saying someone is using the version just-before the one they are actually using.
<facubatista> jam, the only difference between the version they claim and the version they really using would be the version file change, though
<jam> facubatista, if you turn every commit into 2 commits, I guess.
<facubatista> but yes, the proper way to do this would be with support of git itself
<facubatista> like "when commiting, after creating the hash, before saving everything, please dump that hash to this file"
<facubatista> jam, reviewing your branch... you created set_workload_version which gets `version` and passes that directly to `self._backend.application_version_set`, which passes that directly to self._run, which passes that directly to subprocess.run
<facubatista> jam, so if I do set_workload_version(5) it will explode super ugly
<facubatista> maybe _run should str() the *args?
<facubatista> (this is beyond your branch, of course, unless we decide "_run must always receive strings", so this would be something to change in your branch)
<jam> facubatista, so the version should be a string, I don't know that silently auto casting the value to a string behind their back is the right way to do it
<jam> I was actually going to put in a RuntimeError checking for it, but didn't feel it was significantly better than a TypeError
<jam> (there is a test that TypeError is raised for non str)
<jam> if you prefer a different error, I'm happy to do so
<Chipaca> jam: maybe the docstring should talk about the type :)
<facubatista> I'm ok with the TypeError, I'm even ok with the message itself, I'm scared about the traceback
<facubatista>   File "/usr/lib/python3.6/subprocess.py", line 1295, in _execute_child
<facubatista>     restore_signals, start_new_session, preexec_fn)
<facubatista> TypeError: expected str, bytes or os.PathLike object, not int
<Chipaca> jam: also, just noticed, "[...] show in the output of 'juju status'"  should probably be 'shown'
<facubatista> did we consider as a group about type hinting the exposed-to-the-developer interfaces?
<jam> facubatista, Chipaca said that he had investigated some of the alternatives for how we do docstrings, etc. Part of that was whether we wanted to do type hints
<jam> as it changes how we do docstrings if we're using type hints.
<Chipaca> yep
<Chipaca> i myself quite like them
<Chipaca> but we're still to have that discussion
<Chipaca> I'll add something in the calendar right now
<jam> facubatista, does that mean we need top level error catching in the framework for nicer rendering?
<facubatista> jam, maybe, it's a our-pain/user-friendliness balance
<facubatista> (however, we *may* get that for free, after some automatic checking or something, if we have type hints in the "borders")
<jam> Chipaca, changed from ", show" to "; shown" which I think is more correct.
<jam> facubatista, the flip side is figuring out *who* is passing in an int, and that's hard to tell without the traceback
<facubatista> jam, in my mind, the best user experience we can get here is that if you call set_workload_version with not a string, we should fail with something like TypeError("The workload version must be a string")
<facubatista> jam, from that POV, what do you mean "who is passing an int"?
<jam> facubatista, I'm happy to put that check into the method
<Chipaca> facubatista, jam, it could do try/except TypeError as e: raise TypeError("The workload version...") from e
<facubatista> jam, this is more a generic question than a specific request -- are we as a group considering this extra work for user friendliness? etc
<Chipaca> facubatista: I don't think user friendliness should be thought of as 'extra work' :)
<facubatista> Chipaca, well, we should check the rest of the public methods, then
<jam> facubatista, Chipaca : updated the PR with a check. is it better to catch TypeError, or just do isinstance() ?
<jam> facubatista, I do think giving users helpful guidance about how to use the interfaces is very much something we should do
<facubatista> yeap, me too, that's why I brought the subject here
<Chipaca> jam: I lean slightly towards the "try and catch" over the "check beforehand" camp, but not strongly
<Chipaca> jam: but, maybe, in this case, checking is more what we need -- what happens if you try to set the version to "--help" ?
<jam> Chipaca, 'juju status' says '--help'
<jam> its the way you get SOS out
<jam> you hack your charms to say "--help --meeeeeee"
<Chipaca> jam: application-version-set won't try to print its help?
<jam> Chipaca, interesting question. I'm not sure. but likely you could be correct
 * Chipaca releases software with a version of --help all the time
<Chipaca> :-p
<jam> Chipaca, I came across an interesting thing, where various websites like Banks have a secret link that lets you get domestic abuse help
<jam> without "leaving the site"
<Chipaca> interesting
<facubatista> # application-version-set --help
<facubatista> Usage: application-version-set <new-version>
<facubatista> ...
<Chipaca> facubatista: and 'application-version-set -- --help' ?
<facubatista> Chipaca, worked
<facubatista> App             Version  Status       Scale  Charm           Store       Rev  OS      Notes
<facubatista> my-super-charm  --help   unknown          1  my-super-charm  local        18  ubuntu
<Chipaca> sounds like that's what we want to do
<Chipaca> also, that's one for a "spread" suite (^ niemeyer)
<Chipaca> as in, if we care about this (and i do), we'd want to have a test to make sure things don't break down the road (if juju changes options parser for ex)
<Chipaca> lunch!
 * Chipaca â lunch
<jam> Chipaca, so I'm happy to put in '--', but I'd caution around testing every possible interaction between many different components.
<facubatista> maybe a change to include after we have the integration testing in place
<jam> Chipaca, they fixed the 'tiled' layout
<jam> It now does show everyone in little windows
<jam> (at least in a group of 7, I see 6 windows)
<facubatista> jam, wee
<jam> and it was the default layout when joining
<Chipaca> jam: i think they were rolling it out, because yesterday in the mgmt call some people had it and some didn't
<Chipaca> sadly for me, i didn't :)
<Chipaca> here's hoping!
<Chipaca> jam: firefox?
<Chipaca> hah, github just threw a 500 trying to show that pr
<Chipaca> at least it's a cute 500
<jam> Chipaca, yeah, ff
<Chipaca> ooh, it's all 500 all the way down
<jam> Chipaca, are you sure it isn't you ?
<Chipaca> jam: https://github.com/canonical/operator/pulls is 500ing, for ex
<jam> your machine is now triggering 500 in *remote* machines
<Chipaca> â¦ and now it's back
<jam> Chipaca, wfm
<jam> :)
<Chipaca> jam: I MUST NOT ABUSE THIS POWER
<Chipaca> jam: given other people's grumbling elsewhere about this, it's not just me though
<Chipaca> maybe it's all argentine-born people, because that's the one thing in common afaict
<jam> Chipaca, yeah, other people in Juju were saying github is slow
 * Chipaca hangs out in strange places
<niemeyer> I'm taking a break to go out into the sun for a while..
<mthaddon> hi folks, what's the right way to ask for review for a charm we've written? - not urgent in any way, would just like to get feedback from charmcraft team about what we've done and what could be done better
<Chipaca> mthaddon: <3
<Chipaca> mthaddon: not sure, can i get back to you on that?
<mthaddon> yep, no problem
<Chipaca> mthaddon: is this charm public?
<mthaddon> it is, yep
<Chipaca> thinking of something code-review-ish
<Chipaca> anyhoo, need to raise it with the team
<mthaddon> https://jaas.ai/u/wordpress-charmers/wordpress and https://code.launchpad.net/charm-k8s-wordpress for the code
 * Chipaca afk for a while
 * facubatista bb ~1h
 * facubatista is back
 * facubatista eods
<niemeyer> Have a good evening, Facundo
<facubatista> niemeyer, thanks! see you tomorrow
<niemeyer> o/
#smooth-operator 2020-04-24
<Chipaca> mo'in!
<niemeyer> Morning Chipaca, greetings all
<Chipaca> laptop is once again stuck at 800MHz, I'm going to have to take it apart and unplug the battery again :-|
<Chipaca> probably on the weekend
<Chipaca> so, to support dispatch being a script that calls us, while still expecting us to actually *handle* the dispatch, we need to drop looking at argv to know whether we're dispatch and just look at the env var
<Chipaca> to me that means we should set an env var, or clear an env var, to catch us if we try to exec ourselves again (e.g. if somebody's copied charm.py instead of symlinking it)
<Chipaca> this does not seem too bad to me
<niemeyer> Chipaca: We can't drop the former approach for compatibility, but yeah, we need to make it configurable so that the call site for dispatch can use the env
<Chipaca> niemeyer: 'the former approach', you mean looking at argv when we're _not_ dispatch?
<Chipaca> that seems fine
<niemeyer> Chipaca: Yeah, what we have atm
<Chipaca> niemeyer: on a completely separate track, i'm waiting to hear back but it's likely the 'ops' package on pypi is not considered 'abandoned'. If that's the case, what's our fallback?
<niemeyer> Chipaca: We can register something related and still keep the Python module name
<niemeyer> Chipaca: We should circle back to see how to press further on obsolescence of the name, though
<niemeyer> Chipaca: It's certainly abandoned for long
<Chipaca> niemeyer: pypi has a definition of abandoned that is not just 'has it seen a release'
<Chipaca> working on it, in any case
<niemeyer> Chipaca: Sure, but that seems like a valid case of that for all metrics I can imagine
<mup_> Issue operator#240 opened: Shared StoredState between kubernetes workload pods <Created by davigar15> <https://github.com/canonical/operator/issues/240>
<jam>  niemeyer, Chipaca : maybe they use the metric "has it been installed lately" and most of us have accidentally installed it recently :)
<Chipaca> that is one of the things they look at, yes
<davigar15> Chipaca: Dmitrii-Sh and I have found this bug: https://github.com/canonical/operator/issues/240
 * Chipaca looks
<Chipaca> youch
<Chipaca> davigar15: out of ignorance, what creates the symlink from agents/<unit>/charm to agents/<app>/charm ?
<Chipaca> is that juju itself?
<Chipaca> niemeyer, jam, ^^
<jam> Chipaca, most likely Juju, sounds like on K8s every unit shares the charm dir, which we didn't expect
<Chipaca> jam: isn't it fairly common to use the charm dir to store stuff, like keys and the like?
<jam> Chipaca, so in other discussions I've had, you *shouldn't* use the charm_dir, we've discussed trying to make it read-only in Juju, but I do think it gets uses
<jam> used
<Chipaca> jam: so we're doing something we know we shouldn't?
<jam> Chipaca, I don't think we've expressly discussed it, but some things like "why does the state sometimes end up in the application pod" is because we put stuff in the charm dir, which Juju *also* manages with things like charm upgrades.
<jam> Chipaca, I don't think from Juju's end we've clearly stated to charms "here's the directory you should use for your runtime files" which means they use $PWD which is charm_dir
<Chipaca> jam: yeah, i was about to ask, how _can_ a charm grab a directory for its own use?
<jam> Chipaca, ultimately the charm is the root authority on the machine it is setting up. so it is mostly that we should have a well defined recommendation
<jam> it gets slightly hairier if you install 2 apps to the same machine, who 'wins'. And on K8s there is only 1 operator which is responsible for all charm units.
<mup_> Issue operator#241 opened: _TestingModelBackend incorrectly raises KeyError on missing relation <Created by gnuoy> <https://github.com/canonical/operator/issues/241>
<mup_> PR operator#242 opened: Stop test harness relation_ids raising KeyError <Created by gnuoy> <https://github.com/canonical/operator/pull/242>
<jam> anyway, not really here, just passing by, heading back to other activities
<jam> Chipaca, #241 should be fixed by #196, IIRC
<mup_> Issue #241: _TestingModelBackend incorrectly raises KeyError on missing relation <Created by gnuoy> <https://github.com/canonical/operator/issues/241>
<mup_> PR #196: Factor out the Harness changes for test_model.py <Created by jameinel> <https://github.com/canonical/operator/pull/196>
<Chipaca> niemeyer: could you re-review #196 ? i think jam addressed your points
<mup_> PR #196: Factor out the Harness changes for test_model.py <Created by jameinel> <https://github.com/canonical/operator/pull/196>
<Chipaca> davigar15: is that bug blockig your work?
<davigar15> Chipaca: No, I have a workaround for it :-)
<Chipaca> davigar15: ok
<Chipaca> then i'm going to take a bit of time out to see why my battery is draining despite being plugged in (and my cpu stuck at 800mhz)
 * Chipaca bbiab
<davigar15> Basically I needed to update the state of the unit with the data retrieved from the relation_data, and then emit an event. Since it's auto-updated :P I just emit always the event in the relation_changed
<davigar15> Chipaca: Is there a way to install python deps in the charm (similar to wheelhouse.txt)
<davigar15> ?
<niemeyer> davigar15: There will be.. right now this process is a bit manual. We have one pattern described in the README in the repository
<niemeyer> davigar15: We'll have a full building process and associated publication communication with the store
<davigar15> niemeyer: But that procedure is for adding deps from git repositories, right?
<mup_> PR operator#242 closed: Stop test harness relation_ids raising KeyError <Created by gnuoy> <Closed by gnuoy> <https://github.com/canonical/operator/pull/242>
<niemeyer> davigar15: Yes, but the lib directory may have any content on it, and it's added to sys.path
<niemeyer> davigar15: Again, it's a bit manual.. we're working on the build tool
<facubatista> Muy buenos dÃ­as a todos!
<niemeyer> facubatista: Hey hey, good Friday
<facubatista> hola niemeyer, thanks!
<Chipaca> brb, rebooting once again
<Chipaca> facubatista: ð!
<Chipaca> it's a bummer that 'juju ssh' doesn't populate JUJU_ things :-/
<niemeyer> Chipaca: It can't.. it doesn't have an agent behind it
<niemeyer> Chipaca: That is, it's not in a hook context
<niemeyer> Chipaca: That's what debug-hook / debug-code are for, and those do set it.. it's also done via ssh
<Chipaca> niemeyer: debug-hook with no hooks, does that mean all, or none?
<niemeyer> Chipaca: all
<niemeyer> Chipaca: none wouldn't make much sense (debug nothing?)
<facubatista> juju noop
 * Chipaca has a lot of fun pronouncing 'noop' as /n(j)uËp/
<Chipaca> I'd like to propose a breaking change: make the 'short' form observe look for _on_foo instead of of on_foo
<Chipaca> (or, nuke the short form)
<niemeyer> Chipaca: I vote for the latter, but we need a good way to notify people of the coming change
<niemeyer> Chipaca: I'd also vote to do that at the same time that we promise API stability
<niemeyer> Chipaca: We've done too many small breaking changes recently. They were good, but I think the time is approaching where we cannot afford to break any code anymore
<Chipaca> niemeyer: I'd like to start having releases, too :)
<niemeyer> Chipaca: Uhhh.. that's too advanced :P
 * Chipaca giggles at 'juju run --operator'
<mthaddon> davigar15: https://github.com/canonical/operator/issues/156 fwiw
<Chipaca> mthaddon: BTW, what're you doing that makes it better to have a function do that instead of doing it at the package level?
<mthaddon> Chipaca: sorry, not sure I understand the question - how do you mean?
<niemeyer> I think stub and mthaddon understand that this is an ugly hack, but maybe we should add a note to the issue as well pointing that out
<niemeyer> If you want to do something like that, do it on the install event, not at package level
<Chipaca> mthaddon: I mean, https://git.launchpad.net/charm-k8s-wordpress/tree/src/wordpress.py#n14
<Chipaca> it means every single function starts with requests = import_requests()
<mthaddon> Chipaca: ah, it was just that we were needing to do that in a few places so we broke it out into a functionn
<mthaddon> Chipaca: yeah, we could have just done this at the package level too - either way felt less than ideal though
<Chipaca> oh, agreed it's ugly, but doing it in a function is ugly _and_ awkward :-D
 * Chipaca knows it's not like you have a nice option, here
<niemeyer> I've added a note
<niemeyer> Chipaca: There's a nice option in fact
<niemeyer> Chipaca: There's an event called "install", which happens to be a good place to have "apt install"
<Chipaca> niemeyer: did you see my request that you re-review #196?
<mup_> PR #196: Factor out the Harness changes for test_model.py <Created by jameinel> <https://github.com/canonical/operator/pull/196>
<niemeyer> Chipaca: I did.. I have not done it because jam is off and facundo is not. Have been working on something else instead, as we'll discuss in 3 minutes
<Chipaca> niemeyer: "do it on install" does not fix having to make the name available outside of _on_install though
<mthaddon> niemeyer: it also needs to be in a charm upgrade too, otherwise you might never call it again though I think?
<Chipaca> at which point the approach in https://git.launchpad.net/charm-k8s-wordpress/tree/src/wordpress.py#n14 seems to be better
<niemeyer> Chipaca: If the import and name is used inside functions, that's not a problem
<niemeyer> Chipaca: WIll have a look after the standup
<Chipaca> mthaddon: WRT reviewing that charm, can you create an issue in canonical/operator for it? including links to the code :)
<mthaddon> sounds good, will do - thx
<Chipaca> mthaddon: we're trying to get a process for this set up, i'll write it up sometime once we agree
<mup_> Issue operator#243 opened: Review of wordpress kubernetes charm <Created by mthaddon> <https://github.com/canonical/operator/issues/243>
<mthaddon> thx, no hurry from our side, but any feedback appreciated
<niemeyer> mthaddon: Are you available for a call just now?
<mthaddon> niemeyer: yep
<niemeyer> mthaddon: https://meet.google.com/fqw-mdqc-dsf
<mthaddon> niemeyer: can you take a look at https://docs.google.com/document/d/1D-DA3vJRRaauNc2bAJqLTKtAMTTP6tdj5HmOz-rasJI/ and let me know if I've captured it?
<Chipaca> facubatista: did you see my message about #233?
<mup_> PR #233: Execute the registered methods to events under PDB if proper envvar is set <Created by facundobatista> <https://github.com/canonical/operator/pull/233>
<facubatista> Chipaca, I didn't, but now I saw it
<facubatista> Chipaca, niemeyer, proposed now
 * facubatista -> lunch
<Chipaca> facubatista: nice
<mup_> PR operator#244 opened: Draft of the ops library support <Created by niemeyer> <https://github.com/canonical/operator/pull/244>
<Chipaca> niemeyer: nice!
<Chipaca> what a lovely note to wrap a week with :-)
<niemeyer> Chipaca: Take it on! :)
<Chipaca> those two PRs I mean
<niemeyer> Chipaca: This still doesn't support the top-level 'opslib', but that's a nice future PR on top of the first merge
<niemeyer> Chipaca: I did fix it to not import anything before it's actually used
<Chipaca> niemeyer: i noticed
<Chipaca> that's good
<niemeyer> Chipaca: With the nice side effect that now import errors are reported normally
<Chipaca> niemeyer: by 'take it on!' you mean I should grab the PR, make any changes I think it needs, and then drive it as if it were mine?
<Chipaca> niemeyer: or just review as normal and then do follow-ups?
<Chipaca> don't want to do the former if you mean the latter, and viceversa
<niemeyer> Chipaca: Yeah, please feel free to just take ownership
<Chipaca> ok, will do
<niemeyer> Chipaca: I don't want to sit on this further and delay it
<niemeyer> Chipaca: Thank you!
<niemeyer> mthaddon: Doc looks great, thanks!
<mthaddon> niemeyer: cool, thx for the suggestions - will incorporate those soon
<mthaddon> niemeyer: that last bullet point for benefits is really nice (being able to break compatibility without breaking old code)
<niemeyer> mthaddon: Yeah, we really need something that allows people to collaborate fast without clashing with or breaking each other
<niemeyer> mthaddon: This looks painless enough I think
<mthaddon> I'll try and find time to write up the charm developer workflow side of things next week as well, just so it's clear (to me at least) how that part works, but for now I'm EOW o/
<Chipaca> mthaddon: have a good one
<Chipaca> i'm going to go see if my knee is up for a run
<mthaddon> Chipaca: I'd check with both knees before you start :)
<Chipaca> then, i'm going to break gustavo's work :-p
<Chipaca> mthaddon: the one on the right votes conservative, it can do one
<niemeyer> Or fix it, with tests! :)
<niemeyer> Have fun folks
 * facubatista is back
<facubatista> I have a silly mistake in my branch, so I got an error on install, it goes to "error state", if it's broken, why the install is retried?
<facubatista> unit-bdv-4: 16:05:44 DEBUG unit.bdv/4.install AttributeError: module 'os' has no attribute 'chcwd'
<facubatista> unit-bdv-4: 16:05:45 ERROR juju.worker.uniter.operation hook "install" (via explicit, bespoke hook script) failed: exit status 1
<facubatista> unit-bdv-4: 16:05:45 INFO juju.worker.uniter awaiting error resolution for "install" hook
<facubatista> unit-bdv-4: 16:05:50 INFO juju.worker.uniter awaiting error resolution for "install" hook
<facubatista> unit-bdv-4: 16:05:51 INFO unit.bdv/4.juju-log Installing blog...
<facubatista> failed: exit status 1 <--
<facubatista> failed: exit status 1 <-- perfect!
<facubatista> juju.worker.uniter awaiting error resolution for "install" hook <-- perfect, will fix, let me get a mate
<facubatista> unit.bdv/4.juju-log Installing blog... <-- wait, why it started again??
<Chipaca> facubatista: yep, hooks that fail get retried by the juju
<facubatista> Chipaca, and I'm failing to update the code between retrials; and it doesn't let me to do a deploy again (it's says it's already deployed)
<facubatista> so the try/error cycle involves removing the app and deploying from scratch
<facubatista> so much lost time
<Chipaca> facubatista: what happens if you ssh in and just cowboy the new charm in place?
<niemeyer> You can ask to not repeat the hook
<niemeyer> At resolve time
<Chipaca> niemeyer: what's "resolve time"
<facubatista> niemeyer, I'm not running "resolve"
<niemeyer> So how do you tell the unit to keep going? I must be out of context and if so sorry for that
<facubatista> niemeyer, I'm not telling it to continue, it retries by itself
<facubatista> I'd want to tell it "ok, please refresh the code and retry that install"
<niemeyer> facubatista: There must be something broken then, or again I don't understand what's happening.. when a hook returns a non zero exit, juju puts it in an error state
<facubatista> niemeyer, it goes to error, yes, it even says "awaiting error resolution", but then retries out of the blue
<niemeyer> Okay, I'm probably outdated then
<niemeyer> This would not happen in the original implementation
 * facubatista eods
<facubatista> and eows!
<facubatista> see you all on Monday, have a nice weekend
<niemeyer> Have a good weekend
 * Chipaca EOWs also
<Chipaca> see y'all monday
#smooth-operator 2020-04-26
<mup_> PR operator#245 opened: use path.open() instead of open(str(path)) <Created by chipaca> <https://github.com/canonical/operator/pull/245>
