| Chipaca | 👋 | 08:17 |
|---|---|---|
| bthomas | Morning Chipaca | 08:26 |
| bthomas | Chipaca: If a hook changes Config will the change persist in the next hook that is called ? Of course I know one can use StoredState for persistence. | 08:27 |
| Chipaca | bthomas: can a hook change config? | 08:28 |
| bthomas | Chipaca: Have not tried or looked into code. I was under the impression it is an associative array. Guessing by your question, it can not. | 08:30 |
| Chipaca | bthomas: we have a test that would fail if you could | 08:30 |
| bthomas | Chipaca: Nice. Thanks for clarifying. Back to StoredState :) | 08:30 |
| bthomas | Chipaca: In https://github.com/canonical/prometheus-operator/pull/3/files Justin added a function _get_image_tag which required mocking in every test. jam felt we may have a problem with "layering" here. I do not see any other alternative than the once I have suggested in the review comment. IMHO the right "layer" for such functionality is lower down the stack, in Juju or Operator Framework. Thoughts ? | 09:50 |
| bthomas | Refering to this comment : https://github.com/canonical/prometheus-operator/pull/3/files#r497074340 | 09:55 |
| facubatista | ¡Muy buenos días a todos! | 10:58 |
| Chipaca | facubatista: 👋❗ | 10:58 |
| Chipaca | bthomas: looking | 10:58 |
| bthomas | नमस्ते facubatista | 10:59 |
| facubatista | hola Chipaca, bthomas :) | 10:59 |
| bthomas | Thanks Chipaca | 11:01 |
| bthomas | Chipaca: you made a suggetion to avoid mentioning class name, for example `ConfigChanged` in the doc string of `ConfigChangedEvent`. I will follow through and update all such instances. However I noticed that for these event classes, the one line summary docstring is usually written as "Represents the `config-canged` hook from Juju". Would it reasonable to alter such lines as "An event triggered on configuration change" then lo | 11:16 |
| bthomas | in the same doc string, metion that this event "invokes the `config_changed` hook". | 11:16 |
| Chipaca | bthomas: in meetings; i'll get back to you when i can | 11:59 |
| bthomas | no problem | 11:59 |
| jam | bthomas, if we were using resources directly, juju would be providing the digest for you to be using. I'm not a huge fan of hitting the docker registry from the charm (it has all sorts of implications for offline deploys, etc) | 12:25 |
| jam | but I can accept it as a "way to get channel like behavior for now" | 12:25 |
| bthomas | jam Thanks for the insight. I appreciate our current constraints. I have responded to Justin's question regarding "layering" by suggesting doing the patching at setUp and tearDown. This should obviate the impact on all tests, if it is acceptable. | 12:30 |
| jam | bthomas, so ideally it would be more something that we could inject into the charm via a parameter, rather than being patched (IMO). (intended to be configured, vs 'look here is a way to get what I want') | 12:58 |
| jam | and then we would have direct tests of the request based behavior, but split out from the other tests | 12:58 |
| bthomas | jam I do agree with you that using a configuration mechanism (a means of injecting) makes testing simpler. My quandary is that the present requirements do require replacing (partially) a config parameter with a dynamically (runtime) generated one. | 13:41 |
| jam | bthomas, so I mentioned on the bug. I think we do want StoredState to prevent unrelated changes from causing us to upgrade a patch version of the application | 13:41 |
| jam | we should confirm with jldev regarding how we get "smooth transitions of application versions" when we have channels | 13:42 |
| jam | and try to mimic that behavior | 13:42 |
| jam | but my understanding is that it isn't "pods are updated automatically when things are pushed to the rocks store" | 13:42 |
| bthomas | got it. Thanks. | 13:43 |
| * bthomas nudges Chipaca regarding rewording the one line descriptions of event classes ^ | 14:24 | |
| bthomas | :) | 14:25 |
| aluria | hi o/ - I had a look to the grafana-operator and prometheus-operator code (to better understand the approach I should take to enable relations between charms) and I have a couple of questions | 14:30 |
| Chipaca | aluria: 👋 | 14:31 |
| Chipaca | shoot | 14:31 |
| aluria | 1) minor doubt: I think the grafana-source interface should be set in metadata.yaml the other way around. I think it doesn't mind from the point of view of sharing data but the prometheus-operator defines grafana-source in "requires" when it should be in "provides", as it sets host/IP and port for the grafana charm to configure the datasource endpoint | 14:32 |
| aluria | (same, grafana-operator should have the grafana-source interface as requires, because it will use "get" methods) | 14:33 |
| aluria | 2) the other Q is that I've seen some code using EventSource and .emit, but I don't quite understand it. I don't see a .observe counterpart and I'm not sure if the way that would work would be similar to the reactive framework use of states (set_flag and then is_flag_set or @when) | 14:35 |
| aluria | in the grafana and prometheus operator charms, I think they only take care of the -joined, -changed and -departed hooks being observe. EventSource is not used | 14:35 |
| aluria | 3) [sorry, bonus Q] the prometheus-operator uses StoredState - iiuc, that should be used in a similar way as unitdata is used in charmhelpers? (make use of previously retrieved data to avoid a call to relation-get, etc.) ? | 14:37 |
| aluria | Chipaca: ^ :) | 14:37 |
| Chipaca | aluria: just in case, could you link to the code you're talking about? | 14:41 |
| aluria | Chipaca: https://github.com/canonical/grafana-operator and https://github.com/canonical/prometheus-operator | 14:46 |
| Chipaca | hmm, justin isn't on irc | 14:47 |
| Chipaca | bthomas: ^ but you're right there :) | 14:47 |
| bthomas | aluria With regard to Q1, it may help to think of Grafana as a "Dashboard Service Provider" and Prometheus as a data source that "requires a dashboard service". | 14:50 |
| bthomas | With regard to Q2 at least as far as Prometheus was concerned there was no need to emit any event. It was suffecient to just handle events emitted by Juju. | 14:52 |
| aluria | bthomas: re Q2, an example of using .emit is here https://github.com/johnsca/interface-mysql/blob/master/interface_mysql.py#L68 and the "on_changed" method... it looks like when the config-changed hook runs, ".emit" will trigger a -relation-changed hook, right? Is that the use case? | 14:52 |
| aluria | re: Q1, ok if that works for you - I usually relate "provides" with the need of "relation-set" calls, while "requires" needs "relation-get" calls | 14:54 |
| bthomas | aluria: a quick looks shows that that charm is emitting an event database_lost because it had a record of a database connection which now seems to have gone. | 14:55 |
| bthomas | aluria: with regard to Q1 you may want to further discuss this with jam and jldev who are experts on the matter. | 14:56 |
| bthomas | With regard to Q3, all I can say is that StoredState is used to persist data across hook invocations. Use it if you have a use case for it. | 14:56 |
| Chipaca | aluria: in general a component (not a charm) would create custom events and emit() them when whatever situation they're modeling happens | 14:58 |
| Chipaca | aluria: users of that component would observe those events if they needed to react to them, but it's fine if they don't | 14:59 |
| Chipaca | same as your charm doesn't need to write handlers for all juju events | 14:59 |
| bthomas | Chipaca: what do you mean by "component" as opposed to charm. | 14:59 |
| Chipaca | aluria: a database interface for example | 14:59 |
| Chipaca | aluria: interfaces, in general | 15:00 |
| Chipaca | aluria: i have seen some charms creating custom events and emiting them instead of just calling a function, but it's a bad smell | 15:00 |
| Chipaca | ie it wouldn't pass review | 15:00 |
| aluria | that would be similar to interfaces in the reactive framework, when they use set_flag and clear_flag? | 15:01 |
| aluria | good to know re: emitting vs calling a func directly | 15:01 |
| justinclark | Chipaca and aluria I'm here for any Grafana-specific questions now. Sorry for not being on here sooner | 15:14 |
| Chipaca | justinclark: 1 sec | 15:21 |
| Chipaca | <aluria> 1) minor doubt: I think the grafana-source interface should be set in metadata.yaml the other way around. I think it doesn't mind from the point of view of sharing data but the prometheus-operator defines grafana-source in "requires" when it should be in "provides", as it sets host/IP and port for the grafana charm to configure the datasource endpoint | 15:21 |
| Chipaca | <aluria> (same, grafana-operator should have the grafana-source interface as requires, because it will use "get" methods) | 15:21 |
| aluria | justinclark: hey, sorry was on a call - this is the backlog https://pastebin.ubuntu.com/p/jGpsppkjHV/ | 15:21 |
| Chipaca | <aluria> 2) the other Q is that I've seen some code using EventSource and .emit, but I don't quite understand it. I don't see a .observe counterpart and I'm not sure if the way that would work would be similar to the reactive framework use of states (set_flag and then is_flag_set or @when) | 15:21 |
| Chipaca | <aluria> in the grafana and prometheus operator charms, I think they only take care of the -joined, -changed and -departed hooks being observe. EventSource is not used | 15:21 |
| Chipaca | <aluria> 3) [sorry, bonus Q] the prometheus-operator uses StoredState - iiuc, that should be used in a similar way as unitdata is used in charmhelpers? (make use of previously retrieved data to avoid a call to relation-get, etc.) ? | 15:21 |
| aluria | oh, that will also work :D | 15:22 |
| Chipaca | justinclark: that's all the Qs :) | 15:22 |
| Chipaca | heh, sorry | 15:22 |
| aluria | Chipaca: thx for the info re: interfaces and the use of .emit - I have to review that and find what is the suggested approach. I'll have a close look to the database interface (shared before), the prometheus interface (https://git.launchpad.net/interface-prometheus/tree/operator.py#n74) and see what it fits me in my current case | 15:24 |
| Chipaca | justinclark: wrt the 'v' prefix, you could use packaging instead of semver | 15:27 |
| Chipaca | ie from packaging.version import parse | 15:27 |
| Chipaca | parse('1.2.3') == parse('v1.2.3') | 15:27 |
| justinclark | Chipaca thanks - let me take a look. I *really* don't like the current handling of the 'v' prefix | 15:29 |
| * facubatista -> lunch | 15:51 | |
| bthomas | Nice. I am liking package.version. If that works for you justinclark then there may be no need to retain both original tag and parsed tag. Kudos Chipaca | 15:53 |
| justinclark | bthomas it looks like Prometheus has inconsistent version strings sometimes so I think it would still be safer to retain the original tag. But this does get rid of the need to remove 'v' which is really nice. | 16:01 |
| bthomas | justinclark: exactly I did notice that. As mentioned in my review comment that is why I found your solution to keep both original tags and parsed tags an acceptable solution to deal with this evil situation :). But thanks to Chipaca looks like you have a better solution now. | 16:04 |
| justinclark | Well, even with packaging, it is not straightforward how we consistently reconstruct version strings like: "v2.21.0-rc.0" and "0.16.0rc1". Both of which are real examples of tags Prometheus has | 16:06 |
| justinclark | I'm just thinking we should be safe and keep the original string rather than assume we can recreate the original. | 16:07 |
| bthomas | sucks | 16:07 |
| * bthomas start barking loudly | 16:07 | |
| bthomas | justinclark: they seem to be more consistent on recent versions though. | 16:09 |
| bthomas | anyway you decide | 16:09 |
| justinclark | I saw that too. I think I'll keep the current version for now just in case the format changes in the future. Hopefully it will save future developers the headache. | 16:10 |
| bthomas | I am tempted to ask the question on the prometheus forum if the have a well defined format for the version tags ? ;-) | 16:11 |
| Chipaca | a shame packaging doesn't hang on to the original string, but ¯\_(ツ)_/¯ | 16:24 |
| justinclark | Haha that would certainly give me confidence to reconstruct the version, bthomas. | 16:26 |
| bthomas | yep. Again perhaps we should highlight your use case and create an issue on their issue tracker. | 16:27 |
| * justinclark lunch | 16:47 | |
| aluria | cory_fu: hi - would you have ~10min? I'm trying to understand what this does https://github.com/johnsca/charm-gitlab-k8s/blob/c2423f4ec2718a0b5acebe0242f88aa0a1706f39/src/charm.py#L38 | 17:47 |
| aluria | I guess I don't understand the second arg (self), and if it calls the on_database_lost method | 17:47 |
| aluria | I did see the .emit call comes from https://github.com/johnsca/interface-mysql/blob/master/interface_mysql.py#L97 | 17:48 |
| Chipaca | aluria: that no longer works, but when it did it was equivalent to self.framework.observe(self.mysql.on.database_lost, self.on_database_lost) | 18:38 |
| * facubatista eods, will be back on Friday | 19:59 | |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!