search

canonical / cos-proxy-operator

https://charmhub.io/cos-proxy
Apache License 2.0
2 stars 12 forks source link

update charm-relation-interfaces to use prometheus_scrape schema that is compatible with pydantic v2 #141

Closed ca-scribner closed 5 months ago

ca-scribner commented 5 months ago

Issue

Closes #140

Solution

The prometheus_scrape schema includes an optional field for metrics_path. Pydantic's semantics for how a field is marked as "optional" changed between v1 and v2, resulting in Pydantic v2 erroneously interpreting the metrics_path field as required. This PR in charm-relation-interfaces updates the schema to use the pydantic v2 way of defining optional attributes. The current PR updates our dependencies to use this fixed schema.

Also included here is a conversion of a unit test to scenario in order to reproduce the above issue. The previous unit test conceptually should have caught this bug, but because the issue occurred when calling ._get_scrape_configs() (which is only called in __init__) unit tests did not reproduce this bug because Harness reuses the Charm object for all events (eg: __init__ is only called once during harness.begin(), not on every event like in real Juju). By refactoring to scenario, we more closely reproduce true Juju behavior and capture the bug.

Context

see above and #140

Testing Instructions

The new scenario test was introduced first before the charm was fixed to demonstrate that the test captures the error from #140 - see this CI run that demonstrates this. This scenario test should be sufficient to demonstrate the fix works, so if the CI is passing on the PR it should be good.

To reproduce entirely manually, see #140 for instructions.

Upgrade Notes

No special procedure needed. Charm should work once refresh to the latest version.

lucabello commented 5 months ago

Reminder to self: we don't pin charm-relation-interfaces, so this PR is pulling the schema update already :)