MarkMaglanajam: 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/103:59
MarkMaglanaoh and good morning!03:59
jammorning MarkMaglana04:08
jamthanks for the suggestion04:08
MarkMaglanajam: no worries! hope you and the rest of the crew had a great weekend.04:09
stubMight 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 snapsho04:16
stubt")] if show_none or t[1] is not None]))' ./agents/unit-mattermost-1/charm/.unit-state.db04:16
stubWhich 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
jamstub, 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
jamstub, I'd be pretty curious why they want to read raw snapshot data rather than use the objects in memory05:52
jamespecially given things like "juju debug-code" to let you drop into a debugger during your hook execution05:52
stubTrue, so we might want a CLI tool available to dump things05:52
stubPeople like to drop to lower levels sometimes, when they don't understand why the higher levels are giving them the answers they are giving them05:53
stubSay, objects existing in RAM but not being persisted to the DB would point to unexpected termination before commit05:55
stub(whoops, maybe that 'kill -1' wasn't what I meant to do)05:56
mupIssue 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
mupIssue 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
Chipacamorning all08:22
jammorning Chipaca08:26
jamChipaca, https://github.com/relaxdiego/charm-k8s-alertmanager/issues/1 fyi10:15
facubatistaMuy buenos días a todos!11:30
vgrevtsevhi 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
jamvgrevtsev, so Harness has the functionality to work around the problem with registering a class with multiple frameworks.11:31
jamYou can do that work yourself11:31
jamor you can use Harness which does it for you11:31
mupIssue operator#310 opened: hooks/install ran, but other hooks not expanded <Created by stub42> <https://github.com/canonical/operator/issues/310>11:31
vgrevtsevjam: 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
jamvgrevtsev, no. 309 is that your Adapter code is doing "os.environ['JUJU_MODEL_NAME']" and you don't set the environ variable11:32
vgrevtsevBut why it's not failing 100% of the time?11:33
vgrevtsevI mean, at some point this env var is configured by someone else11:33
jamvgrevtsev, so the 307 failure is because if Harness test runs first, then the other test runs successfully (order of tests run)11:33
jamJUJU_MODEL_NAME failed 100% of the time for me.11:33
jamvgrevtsev, are *you* setting JUJU_MODEL_NAME in your shell and running the test suite?11:33
jamso it works in that shell, but not in the other shell?11:34
vgrevtsevOf course no. `tox -epy37` that's what I'm doing.11:34
vgrevtsevjam: https://paste.ubuntu.com/p/qB6XhXfHQH/11:37
vgrevtsevsame code, same command11:37
vgrevtsevbut different failure points - when it raises the KeyError, it fails on `config_changed.emit()` but it was failing on `harness.begin()` before11:38
jamvgrevtsev, look carefully at the line: test/charm_test.py F..11:38
jamwhen it is ".F." or "..F" it fails with "cannot register"11:39
jambecause it is running your other test first11:39
jamwhen it is "F.." it fails with JUJU_MODEL_NAME because it is running Harness test first11:39
jamvgrevtsev, if you comment out/patch the non-Harness test, then it will always fail with JUJU_MODEL_NAME11:39
vgrevtsevjam: I see what you mean... true, it's failing 100% of the time11:44
jamChipaca, when Juju returns json output, it doesn't pretty format it, but that leads to ugly strings in tests.11:49
jamShould I create a nice string for the test, or should I make it exactly like the Juju output?11:50
Chipacajam: nice strings are fine11:59
Chipacajam: the assumption that the python json parser won't care seems reasonable12:00
jamChipaca, so it also appears that the format of "status-get --application=true" is very different than --application=False, which also wasn't accounted for12:00
Chipacaas 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 hates12:00
Chipaca(yaml is fine with it though :) )12:00
Chipacajam: ooh12:00
Chipacajam: rock on12:01
Chipacawhere's the documentation for juju's relation-created ?12:03
jamChipaca, since it is new in 2.8 maybe nonexistant ?12:03
Chipaca:'( ok12:04
jamChipaca, this looks like the one that should be updated: https://discourse.juju.is/t/charm-hooks/104012:04
Chipacafacubatista: 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
facubatistaChipaca, +1, shouldn't we improve it at some point?12:09
Chipacafacubatista: totally :) i thought you were doing that :D12:09
Chipacafacubatista: actually, do you have the url for that?12:10
* Chipaca doesn't know where he put it12:10
facubatistaChipaca, https://gist.github.com/facundobatista/d3b7de7a624915227de051cba079e3d612:10
Chipacai'm going to grab lunch12:23
mupPR 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
facubatistaChipaca, 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
jamChipaca, 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
jamthat would seem to say it thinks that it "is_dispatch_aware"13:14
jamChipaca, sounds like you broke it by just detecting JUJU_DISPATCH_PATH in os.environ and didn't check that it wasn't called as 'dispatch13:14
jam"we" broke it when we landed your patch since I certainly was looking at the same code and thought it was correct13:15
Chipacajam: but it's not necessarily called as dispatch (in the shim case for ex)13:15
Chipacawhich was why we changed that behaviour :-/13:15
jamChipaca, 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, yes13:18
jamChipaca, I think it was to support the case where 'dispatch' isn't a symlink but is just a shell script13:19
jambut we need a way to know how we were first invoked13: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
jamChipaca, I posted to the bug, maybe it clearer there?13:20
* jam heads to dinner for a bit, will be back for standup13:22
* Chipaca takes a break13:34
mupPR 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
Chipacafacubatista, jam, fix for #310 ^14:22
mupIssue #310: hooks/install ran, but other hooks not expanded <Created by stub42> <https://github.com/canonical/operator/issues/310>14:22
jamChipaca, and the test properly failed without the patch ?14:23
Chipacajam: oh yes14:23
Chipacajam: i was just commenting as much on the bug :)14:23
ChipacaI could also write a TestMainWithNoDispatchButJujuIsDispatchAwareAndScriptsAreCopies for extra fun14:24
Chipacabut didn't feel it added anything14:24
* Chipaca is a firm believer in separable coordinates14:25
mupIssue 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
mupPR 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
Chipacajam: are you around, or have you stepped away?15:31
jamChipaca, I'm back now15:54
Chipacajam: hi again :)15:54
Chipacajam: should I push the change to use a decorator on #311 ?15:55
mupPR #311: ops/model.py: Proper support for status-get <Created by jameinel> <https://github.com/canonical/operator/pull/311>15:55
Chipacajam: also, should it be 'register', or '_register'? :)15:55
Chipacajam: if _register, should it also be _from_name?15:56
jamChipaca, I originally had "_register" but then my editor complained that I was using a private method of another class15:56
Chipacasingle _ is protected, not private :)15:57
jamand "name = None" gives us the value that auto-complete of things annotated StatusBase have a .name attribute15:57
jamwhich all of the actual implementations have15:57
Chipacajam: we could make it name='' so it's always a str, also15:57
Chipacasorry i'm bikeshedding15:57
jamChipaca, I'm happy enough with that15:58
Chipacajam: why would it complain about _register when you're calling it on the class itself though? or was that in the tests15:58
jamChipaca, so I think it is because it isn't inside the class itself, it was at the top level of the module15:59
Chipacaah ok15: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)
jamChipaca, did you already do the work for decorator, or shall I work on it?16:00
jamChipaca, it seems we have general agreement to do it that way16:00
Chipacajam: i've already done it, but it's not a lot of work so as you wish16:00
jamChipaca, if you have it, I'm happy for you to land it16:01
Chipacajam: pushed, waiting for tests16:28
* facubatista -> lunch16:28
jamChipaca, cheers16:28
ChipacaI'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.116:29
Chipacashould we have a changelog in tree?16:29
jamChipaca, some people like it, I can take it or leave it.16:29
ChipacaI'll go without until somebody complains :)16:30
Chipaca'git log' works for me16:30
Chipaca(and gives us the ability to build the changelog retroactively as long as we're good with the tags16:30
Chipacaanyway. I'm off.16:30
Chipacajam: have a great evening!16:30
* facubatista is back16:59
Chipacafacubatista: i can haz review of #311 ?18:28
mupPR #311: ops/model.py: Proper support for status-get <Created by jameinel> <https://github.com/canonical/operator/pull/311>18:28
facubatistaChipaca, sure18:29
* Chipaca disappears in a puff of dinner-making18:29
facubatistaChipaca, done18:30
facubatista(I already reviewed it in the morning, the only thing missing was the "how to register" thing18:30
facubatistaChipaca, 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
Chipacafacundo__: it translates to 'python setup.py develop' i think21:06
mupIssue 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
mupPR 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
mupIssue 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
mupPR 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
mupPR operator#315 opened: bumping ops.__version__ for release 0.6.1 <Created by chipaca> <https://github.com/canonical/operator/pull/315>21:12
Chipacafacundo__: ping21:40
Chipacaok, i'm landing the release bump21:43
mupPR 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)
Chipacastub: 0.6.1 just released for the fix to your #310 (thanks!)21:53
mupIssue #310: hooks/install ran, but other hooks not expanded <Created by stub42> <Closed by chipaca> <https://github.com/canonical/operator/issues/310>21:53
Chipacaand now bed21:53
mupIssue 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!