| MarkMaglana | jam: if you're still looking for a way to review code outside of a PR (i.e. code that's already been merged), I created a sample issue here: https://github.com/relaxdiego/charm-k8s-alertmanager/issues/1 | 03:59 |
|---|---|---|
| MarkMaglana | oh and good morning! | 03:59 |
| MarkMaglana | :-) | 03:59 |
| jam | morning MarkMaglana | 04:08 |
| jam | thanks for the suggestion | 04:08 |
| MarkMaglana | jam: no worries! hope you and the rest of the crew had a great weekend. | 04:09 |
| stub | Might want to revisit the use of storing pickles in the db rather than json or yaml. I'm seeing devs hand around one liners for debug like: root@mattermost-operator-0:/var/lib/juju# python3 -c 'import pickle, pprint, sqlite3, sys ; show_none = False ; print("\n".join(["=== {} ===\n{}\n".format(t[0], pprint.pformat(t[1])) for t in [(row[0], pickle.loads(row[1])) for row in sqlite3.connect(sys.argv[1]).execute("SELECT handle, data from snapsho | 04:16 |
| stub | t")] if show_none or t[1] is not None]))' ./agents/unit-mattermost-1/charm/.unit-state.db | 04:16 |
| stub | Which is not the sort of friendly debugging tip we want on the forums, when you could be using the sqllite cli as intended and it not being an operator framework problem. | 04:17 |
| jam | stub, I agree that is pretty ugly. With the caveats that we are looking to stop having an SQLite database locally anyway, since Juju 2.8 will allow us to store the state in the juju controller. | 05:51 |
| jam | stub, I'd be pretty curious why they want to read raw snapshot data rather than use the objects in memory | 05:52 |
| jam | especially given things like "juju debug-code" to let you drop into a debugger during your hook execution | 05:52 |
| stub | True, so we might want a CLI tool available to dump things | 05:52 |
| stub | People like to drop to lower levels sometimes, when they don't understand why the higher levels are giving them the answers they are giving them | 05:53 |
| stub | Say, objects existing in RAM but not being persisted to the DB would point to unexpected termination before commit | 05:55 |
| stub | (whoops, maybe that 'kill -1' wasn't what I meant to do) | 05:56 |
| mup | Issue operator#265 closed: Auto determining handlers is actually not recommended <Created by jameinel> <Closed by jameinel> <https://github.com/canonical/operator/issues/265> | 07:30 |
| mup | Issue operator#302 closed: AttributeError: 'MattermostCharmEvents' object has no attribute 'db_relation_joined' <Created by jetpackdanger> <Closed by jameinel> <https://github.com/canonical/operator/issues/302> | 07:32 |
| Chipaca | morning all | 08:22 |
| jam | morning Chipaca | 08:26 |
| jam | Chipaca, https://github.com/relaxdiego/charm-k8s-alertmanager/issues/1 fyi | 10:15 |
| facubatista | Muy buenos días a todos! | 11:30 |
| vgrevtsev | hi jam - so re: https://github.com/canonical/operator/issues/307 did I understand correctly that the only way to fix this issue is to re-write ALL of my unit tests with harness? | 11:30 |
| jam | vgrevtsev, so Harness has the functionality to work around the problem with registering a class with multiple frameworks. | 11:31 |
| jam | You can do that work yourself | 11:31 |
| jam | or you can use Harness which does it for you | 11:31 |
| mup | Issue operator#310 opened: hooks/install ran, but other hooks not expanded <Created by stub42> <https://github.com/canonical/operator/issues/310> | 11:31 |
| vgrevtsev | jam: ok, and https://github.com/canonical/operator/issues/309 - you think it has the same root cause as 307? | 11:32 |
| vgrevtsev | (re model name env var) | 11:32 |
| jam | vgrevtsev, no. 309 is that your Adapter code is doing "os.environ['JUJU_MODEL_NAME']" and you don't set the environ variable | 11:32 |
| vgrevtsev | But why it's not failing 100% of the time? | 11:33 |
| vgrevtsev | I mean, at some point this env var is configured by someone else | 11:33 |
| jam | vgrevtsev, so the 307 failure is because if Harness test runs first, then the other test runs successfully (order of tests run) | 11:33 |
| jam | JUJU_MODEL_NAME failed 100% of the time for me. | 11:33 |
| jam | vgrevtsev, are *you* setting JUJU_MODEL_NAME in your shell and running the test suite? | 11:33 |
| jam | so it works in that shell, but not in the other shell? | 11:34 |
| vgrevtsev | Of course no. `tox -epy37` that's what I'm doing. | 11:34 |
| vgrevtsev | jam: https://paste.ubuntu.com/p/qB6XhXfHQH/ | 11:37 |
| vgrevtsev | same code, same command | 11:37 |
| vgrevtsev | but different failure points - when it raises the KeyError, it fails on `config_changed.emit()` but it was failing on `harness.begin()` before | 11:38 |
| jam | vgrevtsev, look carefully at the line: test/charm_test.py F.. | 11:38 |
| jam | when it is ".F." or "..F" it fails with "cannot register" | 11:39 |
| jam | because it is running your other test first | 11:39 |
| jam | when it is "F.." it fails with JUJU_MODEL_NAME because it is running Harness test first | 11:39 |
| jam | vgrevtsev, if you comment out/patch the non-Harness test, then it will always fail with JUJU_MODEL_NAME | 11:39 |
| vgrevtsev | jam: I see what you mean... true, it's failing 100% of the time | 11:44 |
| jam | Chipaca, when Juju returns json output, it doesn't pretty format it, but that leads to ugly strings in tests. | 11:49 |
| jam | Should I create a nice string for the test, or should I make it exactly like the Juju output? | 11:50 |
| Chipaca | jam: nice strings are fine | 11:59 |
| Chipaca | jam: the assumption that the python json parser won't care seems reasonable | 12:00 |
| jam | Chipaca, so it also appears that the format of "status-get --application=true" is very different than --application=False, which also wasn't accounted for | 12:00 |
| Chipaca | as long as it's valid json mind you; one problem with pretty-printing it is that you might end with , at the end of an object, which json hates | 12:00 |
| Chipaca | (yaml is fine with it though :) ) | 12:00 |
| Chipaca | jam: ooh | 12:00 |
| Chipaca | jam: rock on | 12:01 |
| Chipaca | where's the documentation for juju's relation-created ? | 12:03 |
| jam | Chipaca, since it is new in 2.8 maybe nonexistant ? | 12:03 |
| Chipaca | :'( ok | 12:04 |
| jam | Chipaca, this looks like the one that should be updated: https://discourse.juju.is/t/charm-hooks/1040 | 12:04 |
| Chipaca | facubatista: wrt the tutorial gist, ok if i link it from the dev summary? (with a note that when done we'll put it on discourse) | 12:08 |
| facubatista | Chipaca, +1, shouldn't we improve it at some point? | 12:09 |
| Chipaca | facubatista: totally :) i thought you were doing that :D | 12:09 |
| Chipaca | facubatista: actually, do you have the url for that? | 12:10 |
| * Chipaca doesn't know where he put it | 12:10 | |
| facubatista | Chipaca, https://gist.github.com/facundobatista/d3b7de7a624915227de051cba079e3d6 | 12:10 |
| Chipaca | i'm going to grab lunch | 12:23 |
| mup | PR operator#311 opened: ops/model.py: Proper support for status-get <Created by jameinel> <https://github.com/canonical/operator/pull/311> | 12:57 |
| === facundo__ is now known as facubatista | ||
| facubatista | Chipaca, taking in consideration that this meeting was short, why not have the standup at usual time (in 17' from now) so it doesn't get superlate to jam? | 13:13 |
| jam | Chipaca, looking at https://github.com/canonical/operator/issues/310 I see it triggering "run_any_legacy_hook" and seeing that dispatch is just a symlink to itself. | 13:14 |
| jam | that would seem to say it thinks that it "is_dispatch_aware" | 13:14 |
| jam | Chipaca, sounds like you broke it by just detecting JUJU_DISPATCH_PATH in os.environ and didn't check that it wasn't called as 'dispatch | 13:14 |
| jam | "we" broke it when we landed your patch since I certainly was looking at the same code and thought it was correct | 13:15 |
| Chipaca | hmmm | 13:15 |
| Chipaca | jam: but it's not necessarily called as dispatch (in the shim case for ex) | 13:15 |
| Chipaca | which was why we changed that behaviour :-/ | 13:15 |
| jam | Chipaca, so the issue is that JUJU_DISPATCH_PATH is set, which is the only check we are currently using, but we aren't being called from dispatch, yes | 13:18 |
| jam | Chipaca, I think it was to support the case where 'dispatch' isn't a symlink but is just a shell script | 13:19 |
| jam | but we need a way to know how we were first invoked | 13:19 |
| jam | (for example, if sys.argv[0] has 'hooks/*' then we do need to create the links, or if 'dispatch' doesn't exist, etc.) | 13:20 |
| jam | Chipaca, I posted to the bug, maybe it clearer there? | 13:20 |
| * jam heads to dinner for a bit, will be back for standup | 13:22 | |
| * Chipaca takes a break | 13:34 | |
| mup | PR operator#312 opened: ops.main: only do _init_dispatch() if dispatch exists <bug> <Created by chipaca> <https://github.com/canonical/operator/pull/312> | 14:22 |
| Chipaca | facubatista, jam, fix for #310 ^ | 14:22 |
| mup | Issue #310: hooks/install ran, but other hooks not expanded <Created by stub42> <https://github.com/canonical/operator/issues/310> | 14:22 |
| jam | Chipaca, and the test properly failed without the patch ? | 14:23 |
| Chipaca | jam: oh yes | 14:23 |
| jam | lgtm | 14:23 |
| Chipaca | jam: i was just commenting as much on the bug :) | 14:23 |
| Chipaca | I could also write a TestMainWithNoDispatchButJujuIsDispatchAwareAndScriptsAreCopies for extra fun | 14:24 |
| Chipaca | but didn't feel it added anything | 14:24 |
| * Chipaca is a firm believer in separable coordinates | 14:25 | |
| mup | Issue operator#313 opened: If ops is installed, the test run picks up the wrong one <Created by chipaca> <https://github.com/canonical/operator/issues/313> | 14:54 |
| mup | PR operator#314 opened: test_main: set up PYTHONPATH so the sub-process picks up this ops <Created by chipaca> <https://github.com/canonical/operator/pull/314> | 15:28 |
| Chipaca | jam: are you around, or have you stepped away? | 15:31 |
| jam | Chipaca, I'm back now | 15:54 |
| Chipaca | jam: hi again :) | 15:54 |
| Chipaca | jam: should I push the change to use a decorator on #311 ? | 15:55 |
| mup | PR #311: ops/model.py: Proper support for status-get <Created by jameinel> <https://github.com/canonical/operator/pull/311> | 15:55 |
| Chipaca | jam: also, should it be 'register', or '_register'? :) | 15:55 |
| Chipaca | jam: if _register, should it also be _from_name? | 15:56 |
| jam | Chipaca, I originally had "_register" but then my editor complained that I was using a private method of another class | 15:56 |
| Chipaca | aw | 15:56 |
| Chipaca | single _ is protected, not private :) | 15:57 |
| jam | and "name = None" gives us the value that auto-complete of things annotated StatusBase have a .name attribute | 15:57 |
| jam | which all of the actual implementations have | 15:57 |
| Chipaca | jam: we could make it name='' so it's always a str, also | 15:57 |
| Chipaca | sorry i'm bikeshedding | 15:57 |
| jam | Chipaca, I'm happy enough with that | 15:58 |
| Chipaca | jam: why would it complain about _register when you're calling it on the class itself though? or was that in the tests | 15:58 |
| jam | Chipaca, so I think it is because it isn't inside the class itself, it was at the top level of the module | 15:59 |
| Chipaca | ah ok | 15:59 |
| === Chipaca changed the topic of #smooth-operator to: general discussion of the operator framework || github.com/canonical/operator || ops 0.6.0 (pip install ops) || charmcraft 0.1.0 (pip install charmcraft) | ||
| jam | Chipaca, did you already do the work for decorator, or shall I work on it? | 16:00 |
| jam | Chipaca, it seems we have general agreement to do it that way | 16:00 |
| Chipaca | jam: i've already done it, but it's not a lot of work so as you wish | 16:00 |
| jam | Chipaca, if you have it, I'm happy for you to land it | 16:01 |
| Chipaca | jam: pushed, waiting for tests | 16:28 |
| * facubatista -> lunch | 16:28 | |
| jam | Chipaca, cheers | 16:28 |
| Chipaca | I'll take off now, go for my run and dinner and etc, and will come back after that to land this and push out 0.6.1 | 16:29 |
| Chipaca | should we have a changelog in tree? | 16:29 |
| jam | Chipaca, some people like it, I can take it or leave it. | 16:29 |
| Chipaca | I'll go without until somebody complains :) | 16:30 |
| Chipaca | 'git log' works for me | 16:30 |
| Chipaca | (and gives us the ability to build the changelog retroactively as long as we're good with the tags | 16:30 |
| Chipaca | ) | 16:30 |
| Chipaca | anyway. I'm off. | 16:30 |
| Chipaca | jam: have a great evening! | 16:30 |
| * facubatista is back | 16:59 | |
| Chipaca | facubatista: i can haz review of #311 ? | 18:28 |
| mup | PR #311: ops/model.py: Proper support for status-get <Created by jameinel> <https://github.com/canonical/operator/pull/311> | 18:28 |
| facubatista | Chipaca, sure | 18:29 |
| Chipaca | tks | 18:29 |
| * Chipaca disappears in a puff of dinner-making | 18:29 | |
| facubatista | Chipaca, done | 18:30 |
| facubatista | (I already reviewed it in the morning, the only thing missing was the "how to register" thing | 18:30 |
| facubatista | ) | 18:30 |
| facubatista | Chipaca, the "-e" that you put in operator's requirements.txt file, theorically is the "--editable" option of "pip install", right? how that is translated to "get dependencies from setup.py"? | 18:38 |
| Chipaca | facundo__: it translates to 'python setup.py develop' i think | 21:06 |
| mup | Issue operator#288 closed: StatusBase are registered on instantiation not definition <Created by jameinel> <Closed by chipaca> <https://github.com/canonical/operator/issues/288> | 21:09 |
| mup | PR operator#311 closed: ops/model.py: Proper support for status-get <Created by jameinel> <Merged by chipaca> <https://github.com/canonical/operator/pull/311> | 21:09 |
| mup | Issue operator#310 closed: hooks/install ran, but other hooks not expanded <Created by stub42> <Closed by chipaca> <https://github.com/canonical/operator/issues/310> | 21:10 |
| mup | PR operator#312 closed: ops.main: only do _init_dispatch() if dispatch exists <bug> <Created by chipaca> <Merged by chipaca> <https://github.com/canonical/operator/pull/312> | 21:10 |
| mup | PR operator#315 opened: bumping ops.__version__ for release 0.6.1 <Created by chipaca> <https://github.com/canonical/operator/pull/315> | 21:12 |
| Chipaca | facundo__: ping | 21:40 |
| Chipaca | ok, i'm landing the release bump | 21:43 |
| mup | PR operator#315 closed: bumping ops.__version__ for release 0.6.1 <Created by chipaca> <Merged by chipaca> <https://github.com/canonical/operator/pull/315> | 21:43 |
| === Chipaca changed the topic of #smooth-operator to: general discussion of the operator framework || github.com/canonical/operator || ops 0.6.1 (pip install ops) || charmcraft 0.1.0 (pip install charmcraft) | ||
| Chipaca | stub: 0.6.1 just released for the fix to your #310 (thanks!) | 21:53 |
| mup | Issue #310: hooks/install ran, but other hooks not expanded <Created by stub42> <Closed by chipaca> <https://github.com/canonical/operator/issues/310> | 21:53 |
| Chipaca | and now bed | 21:53 |
| mup | Issue operator#316 opened: unit state lost following upgrade-charm <Created by jetpackdanger> <https://github.com/canonical/operator/issues/316> | 23:39 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!