jammroning all04:30
=== max__ is now known as Guest85769
Chipacagood morning!08:18
* Chipaca stayed up too late and now needs more coffee08:21
Chipacais there a good reason for _is_our_unit and _is_our_app to be called like that, as opposed to something more unified like _is_ours (or _is_self)?08:40
niemeyerMorning Chipaca08:46
niemeyerChipaca: Reviews mainly.. it wasn't easy to get to those names either :)08:47
jammorning Chipaca and niemeyer08:48
niemeyer"Is ours what?" ""self is something else" and it goes on..08:48
niemeyerMorning jam08:48
jamChipaca, niemeyer : if the parameter of 'juju debug-code' is now '--at', should the env var still be JUJU_BREAKPOINT?08:53
jam'JUJU_DEBUG_AT' or something along those lines feels like it matches better.08:53
jamThe doc is also now significantly out of date, and I'm not sure I was around for all the conversations.08:54
jamI pretty much just understood changing "juju debug-hook --breakpoint" to "juju debug-code --at" but the rest seem to stay the same.08:54
niemeyerSounds reasonable and generic enough to be useful in multiple ways.. can't think of better alternatives right away08:56
Chipacaniemeyer: are you talking about AT or about BREAKPOINT there08:56
Chipacacan be read both ways08:56
niemeyerThe suggestion08:57
Dmitrii-ShChipaca: added a link to the spec doc here https://trello.com/c/vcZTwoOI10:27
Dmitrii-ShI am going to update it with more details10:27
niemeyerWe had an agreement to have 160 as the line length10:31
niemeyerI also asked to have "sandbox" into the git ignore list10:32
niemeyerLooks like both have been undone by 907672c0901168903bf17c94165b034881bd35bd10:32
niemeyerLooking at the diff here https://github.com/canonical/operator/pull/162/files10:34
niemeyerThe new lines on the right are consistently less readable10:34
niemeyerDo you really prefer reading the one on the right?10:35
niemeyerThere are several of those...10:35
niemeyerCan we please revert this PR?10:35
Chipacaniemeyer: 160 is much too wide10:35
niemeyerChipaca: I don't care about the number as much as I care about the result10:36
Chipacaniemeyer: yes, i prefer the one on the right10:36
niemeyerWe can make it 1000 is if it looks better10:36
niemeyerChipaca: Really!? Are you seriously saying you read that completely broken up text and code better than what you have on the left side?10:36
Chipacaniemeyer: and sanbox is still in git ignore10:36
Chipacaniemeyer: it's unclear what sandbox is in the gitignore for, but it's still there10:39
Chipacawithout a comment or anything it's a prime candidate for cleanup10:39
niemeyerChipaca: Both the line length and sandbox have been discussed before you were in the team10:39
niemeyerChipaca: The line length is 160 precisely so that it reads better.. we got several examples and decided that in practice it ends up looking nicer with longer lines than with arbitrarily broken down indentations10:40
niemeyerChipaca: The sandbox is a directory I personally use to put things that should not be in the tree10:40
jamSo for my personal use, I prefer narrower than 160 for line lengths, as I tend to have 2 or 3 files open concurrently (one for the code, one for the tests of the code, sometimes another as a reference for the code I'm implementing)10:46
jamhttps://www.python.org/dev/peps/pep-0008/#maximum-line-length itself recommends up to 99 chars,10:47
jamI don't prefer the 'indent to where the (' opens, as it tends to waste a lot of horizontal space10:47
niemeyerjam: How do broken down lines look like in your terminal?  Does it fit more than 99 chars, or is it broken down further?10:48
jamso I tend to do the return immediately after the (which causes it to indent just a single extra indent level)10:48
niemeyerjam: Because reviews look terrible..  it's neither 99 nor 16010:48
niemeyerjam: Which means we get the manual line breaking, and then the enforced breaking on top of that either way10:48
niemeyerjam: (as we had discussed many months ago)10:49
jamI believe the actual line length discussion predates me as well.10:49
jamIn my editor, it tends to horizontal scroll, in a terminal, it wraps the line10:50
niemeyerjam: Or maybe it just wasn't interesting enough to be talking in the channel.. ;)10:50
niemeyerjam: Okay, so 99 will *also* wrap and horizontal scroll..10:52
niemeyerYeah, just barely.. conversation happened on Oct 3rd, you joined on Oct 2nd10:56
jamfair enough. I remember it being 160, but certainly didn't have a strong rationale for defending it staying there.11:01
niemeyerIt's okay..  the rationale is not super insightful other than we looked at code and it looked worse when indenting too much11:03
niemeyerReading the delta seems to confirm that still..11:03
niemeyerSometimes it's like this:11:04
niemeyerSometimes like that:11:04
niemeyerAnd sometimes this:11:05
niemeyerVersus stuff like this:11:06
niemeyer-                        raise RuntimeError("StoredState shared by {}.{} and {}.{}".format(parent_tname, self.attr_name, parent_tname, attr_name))11:06
niemeyerI easily prefer the latter, maybe because I'm unusual in that I don't appreciate jumping my eyes around  with no semantic involved and do appreciate its tightness. We can discuss further in the standup.11:08
jamso personally, while I don't prefer arbitrarily wrapped differently each time, I do prefer the wrapped form. I think it is because it decreases the linear distance11:08
jamI think there was also an issue with github rendering, at least IIRC that is what Chipaca put the PR up for.11:09
niemeyerThe screenshot I first posted above was from GH11:09
facubatistaMuy buenos días a todos!11:15
facubatistaSo, you decided to go -vvvvvv mode the day I was off? :p11:16
* facubatista has a lot of stuff to read11:16
Chipacafacubatista: it's _because_ you were off11:16
Chipacafacubatista: it's you that's been inhibiting us11:16
* Chipaca just out of a meeting and needs to read up11:17
jamI run a 27" monitor, and while I can go wider, I tend not to maximize browser windows because they are too wide.11:17
jamIn my normal width: https://imgur.com/cSVJFWU the old one wraps but the new one doesn't.11:18
niemeyerjam: To be clear, my point was never that the monitor was wide enough to fit 160 chars11:18
jamIf I max the window, neither wraps https://imgur.com/kfjK3IR11:18
jambut I actually find it hard to review at that size, because the two sides are too far apart11:18
jamit is hard visually to follow from left to right for me11:18
niemeyerjam: The point is rather that this is not a great way to force the formatting of code, because in practice it ends up looking worse in multiple places11:18
jamand if I make it narrow https://imgur.com/1YEpDMK it looks awful in the new ay11:19
niemeyerjam: Extremely long lines can usually be reorganized to not be extremely long by using proper coding practices11:19
niemeyerjam: And we can always *choose* to break lines down where really appropriate11:19
niemeyerjam: What the limit does is to force people to break them down whatever it takes.. and we end up with the stuff mentioned11:19
niemeyerThat's subjective, though.. so let's just raise that up in the next standup and see where the team stands11:21
niemeyerjam: This "narrow" view is actually what I get naturally over here, and I'm on a high-resolution (4k) 14 inch monitor11:22
jamfor whatever reason my 'natural' window size is about 1200x1200 pix. it fits most content cleanly (bugs on launchpad, gogole docs, etc)11:24
niemeyerMy resolution is 2560x1440 (2k actually)11:26
niemeyerI use a tiling WM, but for that sort of content the window is always full screen11:26
jamChipaca, facubatista : I tried to encapsulate the discussions that happened yesterday around "Debugging live charms" and brought up a couple more questions11:32
jamI also have 'juju debug-code' sketched up, testing it now.11:32
facubatistajam, will go there when I finish all the reading here, 5'11:33
facubatistaI don't like this breaking lines style:11:46
facubatista  some_call(foobar,11:46
facubatista            other,11:46
facubatistanot only because the totally wasted space there, but also because it's indented to (75% chance) a weird column11:47
facubatistaand pathologically leads to code like this: https://pastebin.canonical.com/p/fb7XW8yJWx/11:47
facubatistaniemeyer, for me code at the left here is totally broken: https://usercontent.irccloud-cdn.com/file/5g77t1LD/image.png11:48
niemeyerfacubatista: Broken in which sense? You're probably not using that word in ways I normally read it :)11:49
niemeyerDmitrii-Sh: https://github.com/canonical/cockroachdb-operator/pull/1 has an initial review11:50
facubatistathe idea is also to not see it as the right, never, that's why a sensible value for column limit like 99 is thought to have it not too wide to have those line breaks11:50
facubatistaniemeyer, I mean, my eyes can not parse that; the moment I have a line that wraps and gets too long, I lose the identation11:51
niemeyerfacubatista: Agreed.. and that's my main concern, to be clear. 160 is an arbitrary number to prevent such formatting.. I don't have anything locally that would make 160 special.11:51
niemeyerfacubatista: But, that formatting in the right is what I consistently get..11:51
niemeyerI probably need a wider monitor, which is ironic given that I'm the one advocating for wider lines11:52
Dmitrii-Shthanks, also submitted the tls-certificates interface for review https://github.com/canonical/operator/pull/21011:52
niemeyerANyway, lunch time here11:52
niemeyerAnd we have a meeting in just a few11:52
niemeyerSee you soon11:52
Dmitrii-Shneed to figure out what to do about python-cryptography for pull/21011:52
facubatistaaghhh, canonical's pastebin messes with the identation11:54
facubatistaniemeyer, btw, what you see on the right there is also because it's indented where the parenthesis break! that should be like this: http://linkode.org/#A3zKfjmPofCP2jgBR5eo511:55
facubatista(or like the second version there)11:55
jamfacubatista, I also prefer that wrapping11:58
facubatistaniemeyer, stub, regarding the surprising json key: https://bugs.python.org/issue3497212:20
jamfacubatista, "its how the format works", in that it doesn't support ints for keys, not that it treats ints as strings...12:23
facubatistajam, yes, it was surprising for me too! I actually found the issue when going to report the issue, a couple of months ago :)12:24
* facubatista had it fresh because of that12:24
facubatistajam, thanks for the massive editing in the Debugger doc12:28
jamfacubatista, It ended a bit ugly, but I was hoping to show the changes as discussed, and make sure it was clear what was changing.12:29
facubatistajam, yes, it's way better for me to see that, instead of having to read everything and diff current text to what was in my memory12:29
narinderguptamacHello all14:38
facubatistahello narinderguptamac14:39
narinderguptamacfacubatista, was taking to Dmitri about peer relation and got it cleared that any peer event will fire after start event14:41
facubatistanarinderguptamac, nice!14:44
* facubatista -> lunch15:14
crodriguezhello hello15:24
crodriguezanyone tried to attach secrets with this new feature from juju 2.8? https://discourse.juju.is/t/k8s-spec-v3-changes/2698. It doesn't work for me and I'm unsure if it points to juju or the framework itself15:25
crodriguezalso, is there an example somewhere with the import of the k8s yaml manifest in the charm.py, instead of having to write the spec in a yaml format ourselves?  I believe calvin did it for his cdk-cats charm, but I don't think it was fully functional15:28
Chipacacrodriguez: o/!15:37
Chipacacrodriguez: we haven't done the work to support the new secrets thing AFAIK15:38
Chipacacrodriguez: we will shortly15:38
crodriguezChipaca: aah ok then I'll wait before integrating that. Thanks! Is there a newsletter / discourse / etc. where updates, new features, etc are published?15:39
jamChipaca, crodriguez : so any pod spec that they give us we pass on through, I think you're crossing 2 different things.15:41
jamcrodriguez, the first thing that stands out from the thread is making sure your pod spec is version 3, beyond that *I* don't know what the values of the fields should be.15:41
jamcrodriguez, the actual bug you have is in your code, though15:42
jamapplication-mssql: 10:15:33 DEBUG unit.mssql/0.config-changed if self.state.spec != new_spec:15:42
jamthat sais you are looking at 'self.state.spec' but you never initialized the field15:42
jamcrodriguez, you'll want to have something like "self.state.set_default(spec=None)" in your charm's __init__.py15:42
jamsorry def __init__():15:42
jamcrodriguez, I'm actually past EOD here, but if you can put your actual code somewhere we can give you feedback on it.15:43
crodriguezthank you jam! I'll look into the init section of my charm... it might be missing things related to updates have been made to the operator framework. My code is here https://github.com/camille-rodriguez/charm-mssql/tree/add-secret , feel free to look at it tomorr15:45
crodrigueztomorrow*. I'll check the state of the spec like you mentionned15:46
Chipacacrodriguez: what is _write_state in that code?15:48
jamcrodriguez, yeah, I think you used to have initialization in _write_state that was called by on_install, but when you got rid of that, you removed your initialization15:50
crodriguezmhh cause on_install wasn't working until recently. Most of that code was written with the operator framework in late december/early jan, and I'm getting back to it now. I need to clean the on_start if on_install is the right way to go now15:51
jamcrodriguez, so in __init__ having it do self.state.set_default() is the recommended way15:52
jamthat way it is guaranteed to happen even after an upgrade/etc.15:52
crodriguezokay I'll add that15:52
jambut also won't ever overwrite the values that you've actually set.15:52
Chipacajam: you go have a good evening now :)15:54
jamChipaca, well, I have to hang around to get my PR reviewed. Might as well offer some tech support.15:54
crodriguezmmh  "AttributeError: attribute 'set_default' is not stored"15:55
niemeyerHi Camille :)15:56
jamniemeyer, crodriguez : its 'set_default' but it sounds like you have an out-of-date copy of ops15:56
niemeyerI'm not 100% sure as it's been a few weeks, but that's what came to mind when you mentioned the error15:57
jamif you're using submodules you'll need to 'git pull' in the subdir.15:57
niemeyerAh, and jam just proved me wrong.. efficiency! :)15:57
crodriguezin the operator folder? it doesn't have info on the upstream repo15:59
crodriguezI thought it was a command like update submodules or something similar..15:59
Dmitrii-Shcrodriguez: if you have submodules in your repository, you can use this to update all of them: git submodule update --recursive --remote16:03
Dmitrii-Shbut a prerequisite to that is actually adding it16:03
Dmitrii-Shgit submodule add https://github.com/canonical/operator mod/operator16:03
jamDmitrii-Sh, ah, the --remote is the trick that I didn't realize so always just did 'git pull' which worked for me.16:04
crodriguezokay that recursive update command worked, thanks. I moved on to another bug with the framework haha16:04
crodriguezwell with my code16:05
Dmitrii-Shjam: yes it's not very intuitive which reminds me of https://imgs.xkcd.com/comics/git_2x.png16:05
Dmitrii-Shcrodriguez: what is it? Can I help?16:06
jamDmitrii-Sh, git submodule update --help isn't particularly... helpful16:06
Dmitrii-Shjam: heh, it's rather "--quiet"16:07
crodriguezAre we supposed to use the on_install or the on_start hook now? or both? install being for the first deploy, and start for if you restart the nodes?16:07
crodriguezso if I change my pod_spec version to 3, the 'rules' and 'global' keywords are not recognised anymore. Is that from a change in the framework or in juju?16:11
crodriguezlol sorry I'm bombing y'all with questions16:11
Dmitrii-Shhttps://bugs.launchpad.net/juju/+bug/1854635 It's still "start" for now while we use 2.7. This fix was committed to 2.8 for k8s charms to get the install event16:12
Dmitrii-Shbut then "dispatch" will also be in 2.8 https://github.com/juju/juju/pull/11257/files#diff-95b793849a6a72c6122da04f68ecb58616:12
jamcrodriguez, so the framework makes use of on_install to do some initialization stuff, but I wouldn't think you have the information you need to start a pod until after config-changed, and probably around start.16:12
jamDmitrii-Sh, I actually thought that landed in 2.7.4, doesn't look like the bug was updated.16:13
jam"bake and land in 2.7.3" is pretty far away from 2.7.4/5 so not sure where its at.16:13
Dmitrii-Shjam: hmm, I looked at #5 https://bugs.launchpad.net/juju/+bug/1854635/comments/5 for reference, haven't looked at the actual commits16:13
Dmitrii-Shcrodriguez: regarding keywords: you probably mean "roles". There's an example of a spec here in "QA steps" https://github.com/juju/juju/pull/11293 - how different is it to what you have?16:16
crodriguez        spec = {16:18
crodriguez            'version': 2,16:18
crodriguez            'serviceAccount': {16:18
crodriguez                'global': True,16:18
crodriguez                'rules': [16:18
crodriguez                    {16:18
crodriguez                        'apiGroups': ['apps'],16:18
crodriguez                        'resources': ['statefulsets', 'deployments'],16:18
crodriguez                        'verbs': ['*'],16:18
crodriguez                    },16:18
crodriguez                    {16:18
crodriguezso in this spec, if I change version to 3, I get this :16:19
crodriguezsame for unknown field 'rules'16:19
jamcrodriguez, offhand I don't think you're allowed to set global service accounts in a charm. They effect people that aren't in your namespace16:26
jamI'm not the one who has the definitive answer to that, though.16:27
jamI know there were discussions16:27
Dmitrii-Shthe error seems to be coming from the framework though16:27
Dmitrii-Shb'ERROR json: unknown field "global"\n'16:28
Dmitrii-Shhmm, actually that doesn't look like something we'd emit16:28
Dmitrii-ShI just tried using that spec as an input in a unit test and it didn't error out (because we just write to a file)16:29
crodriguezHmm. Okay, I'll look more into that. I need to step out for an hour, will circle back about this. Thanks everyone!16:30
Dmitrii-Shversion: 316:31
Dmitrii-Sh  automountServiceAccountToken: true16:31
Dmitrii-Sh  roles:16:31
Dmitrii-Sh    - global: true16:31
Dmitrii-Sh      rules:16:31
Dmitrii-Sh        - apiGroups: [""]16:31
Dmitrii-Sh          resources: ["pods"]16:31
Dmitrii-Sh          verbs: ["get", "watch", "list"]16:31
Dmitrii-Sh        - nonResourceURLs: ["*"]16:31
Dmitrii-Sh          verbs: ["*"]16:31
Dmitrii-Shcrodriguez: it seems like you need to have "global: true" under roles16:33
Dmitrii-Shand then rules under roles as well16:33
jamDmitrii-Sh, ModelError(e.stderr) means it is coming from Juju16:34
jamDmitrii-Sh, hm, seems v2 is quite a bit different than v3. https://discourse.juju.is/t/updated-podspec-yaml-new-features/212416:37
jamvs https://github.com/juju/juju/pull/1129316:37
Dmitrii-Shjam: I think charms need to specify min-juju-version: 2.8.0 and use a v3 spec if they support v3 only16:39
* facubatista is back16:39
Dmitrii-Shbut this is slightly unfriendly to a user16:39
jamDmitrii-Sh, so AIUI, you can stick with V2, but the goal is to get to something we want to recommend quickly vs a lot of polish on migration16:45
Dmitrii-Shjam: agreed, there is nothing restricting the usage of v3 via pod.spec_set AFAIKS16:46
jamDmitrii-Sh, we need to add 'k8s-spec-set' to the list of 2.8-isms. btw16:46
jamChipaca, ^^ juju is recommended a different name for setting the definition of your pod in 2.816:47
Dmitrii-Shjam: right, the command got renamed16:48
facubatistais there a reason we're not sending DEBUG logs from the framework/charms to juju16:51
niemeyerI don't know what behavior you are observing, but the goal is to make debug logs visible if the user asks to see debug logs16:55
Dmitrii-ShI originally wanted to use DEBUG: https://github.com/canonical/operator/pull/133/commits/2c778fc3f128b088790229bc3cc55bf7f7daedd6#diff-a76991ba26c9c9b312be489400d3f481L2116:56
Dmitrii-ShIIRC the reason not to use it was to encourage people not to use "debug" for everything. It would be nice to retrieve the debug level from the level set for a given model but there's no way to do that yet https://bugs.launchpad.net/juju/+bug/186198716:56
niemeyerWhat we don't want is for the norm to be seeing debug logs at all times, otherwise what we have is really info logs16:56
niemeyerI see debug logs as a fire hose, where even the smallest detail is fine16:57
niemeyerThat's not the kind of thing that should be on by default.. if it is on, we'll learn not to log things at all to not overload people with boring details, and we no longer have debug logs16:58
facubatistaniemeyer, agree with you, that's why I wrote in https://github.com/canonical/operator/issues/198 that the logger needs to be in INFO by default (the user can easily change that) -- the problem is that the *handler* is in INFO, and we should be in DEBUG so if the logger is set to debug, those messages will reach Juju16:58
facubatista*it* should be in DEBUG...16:59
niemeyerAgain, I'm not claiming it's correct.. just that this is what I hope we get16:59
Dmitrii-ShI think we should change the JujuLogHandler level to DEBUG. Otherwise even if one overrides the root logger level in the charm, the messages are still filtered at the handler level17:12
Dmitrii-Shthat would be changing the JujuLogHandler constructor to be like this:     def __init__(self, model_backend, level=logging.DEBUG):17:13
facubatistaDmitrii-Sh, yeap, that's what issue #198 talks about17:19
niemeyerTentatively calling it a day.. o/17:23
tvansteenburgh1Hi everyone. The Canonical k8s team will soon publish (as a preview release) our first k8s charm based on the operator framework. Source code is at https://github.com/charmed-kubernetes/charm-multus. Feedback welcome.17:26
Chipacatvansteenburgh1: ack17:26
Dmitrii-Shcrodriguez: the charm tvansteenburgh1 has a v3 spec example ^ https://github.com/charmed-kubernetes/charm-multus/blob/master/src/charm.py#L87-L12517:28
Dmitrii-Shseems to confirm the theory I had17:28
=== tvansteenburgh1 is now known as tvansteenburgh
crodriguezDmitrii-Sh: thanks for your help. I will compare and adjust to use the v317:38
* facubatista eods20:25
Dmitrii-Shhttps://github.com/canonical/operator/issues/211- found during testing20:38
Dmitrii-Shhttps://github.com/canonical/operator/pull/212 - would be helpful for progressing #21022:40

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!