Code review comment for ~rgildein/charm-telegraf:bug1988312

Revision history for this message
Robert Gildein (rgildein) wrote (last edit ):

> I hope we were talking about the same thing.
> I would like to know can we dectect the error caused by [1] in unit or func
> test?
>
> The simplist way to approve it: apply the patch in [1] and run unit/func
> again. It should failed
>
> If not, it seems we need to add this basic environment used in [2].
>
> series: jammy
> applications:
> ubuntu:
> scale: 1
> charm: ch:ubuntu
> telegraf:
> charm: ch:telegraf
> channel: candidate
> relations:
> - ["ubuntu", "telegraf"]
>
> --
> [1]: https://code.launchpad.net/~hloeung/charm-telegraf/+git/charm-
> telegraf/+merge/428004
> [2]: https://bugs.launchpad.net/charm-telegraf/+bug/1988312

I have talked about the fact that I don't know at all why the MP [1] was
opened at all and therefore whether this is a necessary change at all.
I mentioned here [2], that I was unable to reproduce the error
described in MP [1].

I think it is covered by the unit tests because after [1] the unit test were
failing [3]. At the same time, the functional tests did not work earthed,
thanks to the [4] focal-snap.yaml bundle, which is very similar to the one
in [5].

To be sure, I once again tested the lint, unit and func tests on the
commit from MP [1]. Here are the results:
- lint and unit tests: https://pastebin.ubuntu.com/p/mhFg2HC6Zk/
- func tests: https://pastebin.ubuntu.com/p/p2MrX2YT4W/

To summarized this, I think that changing from
`@when("telegraf.configured")` to `@when("telegraf.installed")` is
fixing both [3] and [5], but I'm not sure if this fixed also issue
mention in [1], since I was not able to reproduced it.

---
[1]: https://code.launchpad.net/~hloeung/charm-telegraf/+git/charm-telegraf/+merge/428004
[2]: https://bugs.launchpad.net/charm-telegraf/+bug/1988312/comments/1
[3]: https://bugs.launchpad.net/charm-telegraf/+bug/1987542
[4]: https://git.launchpad.net/charm-telegraf/tree/src/tests/functional/tests/bundles/base-snap.yaml
[5]: https://bugs.launchpad.net/charm-telegraf/+bug/1988312

« Back to merge proposal