Merge ~rgildein/charm-telegraf:bug1988312 into charm-telegraf:master

Proposed by Robert Gildein
Status: Merged
Approved by: Eric Chen
Approved revision: 8d2bd5d08b48d402a68927cc5a28b9b20cc7140a
Merged at revision: 14575b2671384edaae01e2e7e68d67a518ce3529
Proposed branch: ~rgildein/charm-telegraf:bug1988312
Merge into: charm-telegraf:master
Diff against target: 12 lines (+1/-0)
1 file modified
src/reactive/telegraf.py (+1/-0)
Reviewer Review Type Date Requested Status
prod-jenkaas-bootstack (community) continuous-integration Needs Fixing
Eric Chen Approve
BootStack Reviewers Pending
Review via email: mp+429255@code.launchpad.net

Commit message

fix when case for configure_prometheus_client

Description of the change

Change when case for configure_prometheus_client to `@when("telegraf.installed")`.

fixes: https://bugs.launchpad.net/charm-telegraf/+bug/1988312
lint and unit tests: https://pastebin.ubuntu.com/p/98jbqWWNVM/
functional tests: https://pastebin.ubuntu.com/p/RxGvQbJWDB/

To post a comment you must log in.
Revision history for this message
Robert Gildein (rgildein) wrote :

Functional tests are failing for `bionic-compute` and `focal-compute`
bundle due issue with sudoers.

```bash
# from unit
ubuntu@juju-5ead5f-zaza-12d45d4743f5-8:~$ sudo su
>>> /etc/sudoers.d/telegraf_ovs: syntax error near line 10 <<<
sudo: parse error in /etc/sudoers.d/telegraf_ovs near line 10
sudo: no valid sudoers sources found, quitting
sudo: unable to initialize policy plugin
```

Not sure if this is related, but it worked fine for `jammy-compute`.

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

Result from func, lint and unit tests: https://pastebin.ubuntu.com/p/pZXNcBt7F9/

Revision history for this message
Canonical IS Merge Bot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Approve (continuous-integration)
Revision history for this message
Eric Chen (eric-chen) wrote :

Do we cover the issue in function test? or unit test ?
https://bugs.launchpad.net/charm-telegraf/+bug/1988312
It's a critical bug that we should avoid it.

review: Needs Information
Revision history for this message
Robert Gildein (rgildein) wrote :

This is covered in unit and functional tests.
Unit and functional tests were broken after the MP [1].

The changes from MP [1] was reverted by MP [2] and I
adding them back but improved.

However, I am not sure why and if we need this, as I have
not been able to reproduce the issue described and fixed
by Haw Loeung in MP [1].

---
[1]: https://code.launchpad.net/~hloeung/charm-telegraf/+git/charm-telegraf/+merge/428004
[2]: https://code.launchpad.net/~jneo8/charm-telegraf/+git/charm-telegraf/+merge/429347

Revision history for this message
Eric Chen (eric-chen) wrote :

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

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

Revision history for this message
Eric Chen (eric-chen) :
review: Approve
Revision history for this message
Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision 14575b2671384edaae01e2e7e68d67a518ce3529

Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/reactive/telegraf.py b/src/reactive/telegraf.py
2index bab0236..abbb728 100644
3--- a/src/reactive/telegraf.py
4+++ b/src/reactive/telegraf.py
5@@ -1713,6 +1713,7 @@ def configure_prometheus_client_with_relation(prometheus):
6 clear_flag("endpoint.prometheus-client.changed") # not automatic
7
8
9+@when("telegraf.installed")
10 @when_not("plugins.prometheus-client.configured")
11 def configure_prometheus_client():
12 hookenv.log("Configuring prometheus_client output plugin", level=hookenv.DEBUG)

Subscribers

People subscribed via source and target branches

to all changes: