[IPU] Make sure `revoked` is emitted #80

sed-i · 2022-08-18T19:30:43Z

Issue

Closes #79 .

Solution

add utest(s) for IPU event
return an empty dict if in relation-broken event instead of looking into relation data, which may be out of date.

Context

self.ingress.url was returning old (non-falsey) value even after relation and/or app was removed.
Seems like relation data is not emptied out even when looking from within a relation broken event. This may be a juju feature rather than a bug though/

github-actions · 2022-08-18T19:31:14Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events updated to version 0.3.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape updated to version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

sed-i · 2022-08-18T19:33:28Z

@PietroPasotti I am having trouble with the utest but I'm not sure if it's because of a test bug, lib bug or harness behavior:

The relation is removed with harness, but "removed" is always blank:

INFO     root:ingress_per_unit.py:689 unit: ipu-requirer/0, event: RelationCreatedEvent. removed: set(); changed: set()
INFO     root:ingress_per_unit.py:689 unit: ipu-requirer/0, event: RelationJoinedEvent. removed: set(); changed: set()
INFO     root:ingress_per_unit.py:689 unit: ipu-requirer/0, event: RelationChangedEvent. removed: set(); changed: {'ipu-requirer/0'}
INFO     root:ingress_per_unit.py:689 unit: ipu-requirer/0, event: RelationChangedEvent. removed: set(); changed: set()
INFO     root:ingress_per_unit.py:689 unit: ipu-requirer/0, event: RelationBrokenEvent. removed: set(); changed: set()

and revoke doesn't fire.

github-actions · 2022-08-18T20:54:35Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events updated to version 0.3.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape updated to version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

sed-i · 2022-08-18T20:56:11Z

{} if isinstance(event, BrokenEvent) per @PietroPasotti's suggestion fixed the utest.

sed-i · 2022-08-18T20:59:11Z

lib/charms/traefik_k8s/v1/ingress_per_unit.py

@@ -682,7 +682,7 @@ def _handle_relation(self, event: RelationEvent):
        # we calculate the diff between the urls we were aware of
        # before and those we know now
        previous_urls = self._stored.current_urls or {}  # type: ignore


@rbarry82 would you use _type_convert_stored here?

github-actions · 2022-08-18T23:12:07Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events was already up to date in version 0.3.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape was already up to date in version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

github-actions · 2022-08-18T23:13:54Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events was already up to date in version 0.3.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape was already up to date in version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

github-actions · 2022-08-18T23:28:10Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events was already up to date in version 0.3.
Library charms.observability_libs.v0.juju_topology was already up to date in version 0.2.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape was already up to date in version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

github-actions · 2022-08-18T23:32:30Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events was already up to date in version 0.3.
Library charms.observability_libs.v0.juju_topology was already up to date in version 0.2.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape was already up to date in version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

PietroPasotti

looks good!

lib/charms/observability_libs/v0/juju_topology.py

- Update _stored before events are emitted - Get urls from stored instead of relation data

github-actions · 2022-08-19T16:27:16Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events was already up to date in version 0.3.
Library charms.observability_libs.v0.juju_topology was already up to date in version 0.2.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape was already up to date in version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

github-actions · 2022-08-19T16:37:51Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events was already up to date in version 0.3.
Library charms.observability_libs.v0.juju_topology was already up to date in version 0.2.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape was already up to date in version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

github-actions · 2022-08-19T16:47:15Z

Libraries are not up to date with their remote counterparts. If this was
not intentional, run charmcraft fetch-libs and commit the updated libs
to your PR branch.

stdout

Library charms.harness_extensions.v0.capture_events was already up to date in version 0.3.
Library charms.observability_libs.v0.juju_topology was already up to date in version 0.2.
Library charms.observability_libs.v0.kubernetes_service_patch was already up to date in version 0.6.
Library charms.prometheus_k8s.v0.prometheus_scrape was already up to date in version 0.21.
Library charms.traefik_k8s.v1.ingress has local changes, cannot be updated.
Library charms.traefik_k8s.v1.ingress_per_unit has local changes, cannot be updated.
Library charms.traefik_route_k8s.v0.traefik_route has local changes, cannot be updated.

stderr

rbarry82

Tested manually

PietroPasotti · 2022-09-02T13:23:32Z

@sed-i it seems I didn't pay enough attention while reviewing, or actually, that some changes slipped in that I didn't have a chance to review as they came in after I approved: ce32bd3

either way, there's a problem with these changes:

we renamed urls to _get_urls_from_relation_data, which shouldn't really be a property anymore.
we replaced urls with a function that loads in the urls from stored data, which changes some of the semantics of what's going on, enough to break integration tests in traefik-route. So that's a backwards-incompatible change!

sed-i · 2022-09-02T13:50:19Z

we renamed urls to _get_urls_from_relation_data, which shouldn't really be a property anymore.

Agreed. I noticed that later too. In the IPA PR it is not a property.

we replaced urls with a function that loads in the urls from stored data... break integration tests

Hmm. The change itself still seems correct to me logically. Can you post the link to the failing test? I'm curious.

PietroPasotti · 2022-09-02T13:58:17Z

It wasn't a CI failure, it was local, so no links, but essentially what happened was:
I was writing a tester charm to represent an ingress-requiring-charm like:

class MyCharm(CharmBase):
    def __init__...
        self.unit.status=Blocked()
        ingress = IPURequirer()
        if ingress.is_ready(ingress.relation):
            self.unit.status=Active()

This test was passing before this PR got merged.
reason: is_ready() basically checks if bool(urls).
before this PR, urls would fetch the data in real time from the relation.
after this PR, urls is cached and only contains data if we are processing some events.

However, events are processed AFTER the charm is initialized, so at charm-init-time, that branch wouldn't be selected.

Took me a long time to figure that one out :)
Guess I learned to write charms always 'the proper way', even though it's just a tester charm.

sed-i · 2022-09-02T14:07:12Z

Interesting. I wonder why at charm init time the old url would be non-false-y. And if indeed it was, then was it a mistake?

Add utest for revoked event

0d81fa8

sed-i requested a review from PietroPasotti August 18, 2022 19:30

Return empty dict from broken

5944841

sed-i commented Aug 18, 2022

View reviewed changes

sed-i changed the title ~~Add utest for revoked event~~ [IPU] Make sure revoked is emitted Aug 18, 2022

sed-i requested review from rbarry82 and Abuelodelanada August 18, 2022 21:10

sed-i force-pushed the feature/test_revoked branch from 64bfc64 to 7dbd2aa Compare August 18, 2022 23:13

fetch-lib

2638c16

sed-i force-pushed the feature/test_revoked branch from 7dbd2aa to 2638c16 Compare August 18, 2022 23:27

Bump LIBPATCH

d2f8d48

PietroPasotti approved these changes Aug 19, 2022

View reviewed changes

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

sed-i marked this pull request as ready for review August 19, 2022 13:59

Abuelodelanada approved these changes Aug 19, 2022

View reviewed changes

Ryan's suggestion

ce32bd3

- Update _stored before events are emitted - Get urls from stored instead of relation data

Static fixes

da4d7c8

sed-i force-pushed the feature/test_revoked branch from 5b39af0 to da4d7c8 Compare August 19, 2022 16:46

Fix version in workflows

9cd8a9a

github-actions bot added the Libraries: Out of sync label Aug 19, 2022

rbarry82 approved these changes Aug 19, 2022

View reviewed changes

sed-i merged commit d80dab2 into main Aug 19, 2022

sed-i deleted the feature/test_revoked branch August 19, 2022 18:53

sed-i mentioned this pull request Aug 29, 2022

[IPA] Make sure ingress.url returns an up-to-date value #84

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IPU] Make sure `revoked` is emitted #80

[IPU] Make sure `revoked` is emitted #80

sed-i commented Aug 18, 2022 •

edited

github-actions bot commented Aug 18, 2022

sed-i commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

sed-i commented Aug 18, 2022

sed-i Aug 18, 2022

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

PietroPasotti left a comment

github-actions bot commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

rbarry82 left a comment

PietroPasotti commented Sep 2, 2022

sed-i commented Sep 2, 2022

PietroPasotti commented Sep 2, 2022

sed-i commented Sep 2, 2022

[IPU] Make sure revoked is emitted #80

[IPU] Make sure revoked is emitted #80

Conversation

sed-i commented Aug 18, 2022 • edited

Issue

Solution

Context

github-actions bot commented Aug 18, 2022

sed-i commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

sed-i commented Aug 18, 2022

sed-i Aug 18, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

PietroPasotti left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

rbarry82 left a comment

Choose a reason for hiding this comment

PietroPasotti commented Sep 2, 2022

sed-i commented Sep 2, 2022

PietroPasotti commented Sep 2, 2022

sed-i commented Sep 2, 2022

[IPU] Make sure `revoked` is emitted #80

[IPU] Make sure `revoked` is emitted #80

sed-i commented Aug 18, 2022 •

edited