Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

added config backend to simulate more closely juju's own #787

Merged
merged 4 commits into from
Jul 20, 2022
Merged

added config backend to simulate more closely juju's own #787

merged 4 commits into from
Jul 20, 2022

Conversation

PietroPasotti
Copy link
Contributor

@PietroPasotti PietroPasotti commented Jun 24, 2022 •
edited
Loading

As I was working on #786 I realized also config is not as strict as juju config is. This PR brings the harness' behaviour in line.
From now on, the harness will complain if you try and set a non-existing config key, if you try and set a config key to a value that doesn't match its desired type.

Checklist

  • [ x ] Have any types changed? If so, have the type annotations been updated?
  • [ x ] If this code exposes model data, does it do so thoughtfully, in a way that aids understanding?
  • [ x ] Do error messages, if any, present charm authors or operators with enough information to solve the problem?

QA steps

Added itests for all changed behaviours.

Changelog

  • The harness is now more strict in the way it manages config.
  • Unittests will catch charm code attempting to mutate config (in naive ways).

rwcarlsen
rwcarlsen suggested changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@rwcarlsen rwcarlsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few code structure/design issues I'd like to understand better - comments/questions below.

ops/testing.py Outdated Show resolved Hide resolved
ops/testing.py Outdated Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
@PietroPasotti PietroPasotti requested a review from pengale June 27, 2022 07:10
@PietroPasotti PietroPasotti force-pushed the harness-config-validate branch from 42014cf to e18314d Compare June 27, 2022 07:13
@PietroPasotti
Copy link
Contributor Author

Note that the 011y tests are failing because they do:
image

while they should set to True.

@simskij would that make sense to you?

@PietroPasotti PietroPasotti requested a review from rwcarlsen June 29, 2022 07:15
@PietroPasotti
Copy link
Contributor Author

Note that the 011y tests are failing because they do: image

while they should set to True.

@simskij would that make sense to you?

@simskij reported that the broken tests in o11y charms have been fixed.

rwcarlsen
rwcarlsen approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@rwcarlsen rwcarlsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Just one small note about a potentially redundant error check. Also looks like you'll need to rebase on main and update some conflicts.

ops/testing.py Outdated Show resolved Hide resolved
@rwcarlsen rwcarlsen merged commit bf71c4a into canonical:main Jul 20, 2022
@gregory-schiano gregory-schiano mentioned this pull request Oct 5, 2022
Merged
benhoyt added a commit to benhoyt/traefik-k8s-operator that referenced this pull request Jan 18, 2023
@benhoyt
Fix test issues when using ops 1.5.4
6c367c1 
* test_show_proxied_endpoints_action_no_relations previously had error
  "'ConfigData' object does not support item assignment". This was
  caused by canonical/operator#787 and the fix is to stop setting items
  on Model.config._data directly. Do it with update_config, but
  suppress the config-changed event it triggers.
* test_ingress_app_provider_relate_provide previosly had error
  ops.model.RelationDataTypeError: relation data values must be strings, not <class 'bool'>
  caused by canonical/operator#786 and the fix is to send string values
  instead of Python bools (lowercase as these would be sent to
  relation-set as YAML).
* Fix unit name in test_lib_per_unit* tests (it should be the remote
  app) to silence the warnings about that.

Also remove the unexpected config value "asyncio_mode" in
pyproject.toml's pytest settings to suppress the warning about that.

In addition, tweak some type annotations in the ingress charm libs to
fix these Pyright errors (I haven't done "charmcraft publish-lib" or
similar though):

pyright 1.1.290
/home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress.py
  /home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress.py:218:16 - error: Expression of type "_RelationEventSnapshot" cannot be assigned to return type "dict[Unknown, Unknown]"
    "_RelationEventSnapshot" is incompatible with "dict[Unknown, Unknown]" (reportGeneralTypeIssues)
  /home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress.py:221:25 - error: Argument of type "dict[Unknown, Unknown]" cannot be assigned to parameter "snapshot" of type "_RelationEventSnapshot" in function "restore"
    "dict[Unknown, Unknown]" is incompatible with "_RelationEventSnapshot" (reportGeneralTypeIssues)
  /home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress.py:253:10 - error: Expression of type "IngressPerAppProviderEvents" cannot be assigned to declared type "property"
    "IngressPerAppProviderEvents" is incompatible with "property" (reportGeneralTypeIssues)
  /home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress.py:409:10 - error: Expression of type "IngressPerAppRequirerEvents" cannot be assigned to declared type "property"
    "IngressPerAppRequirerEvents" is incompatible with "property" (reportGeneralTypeIssues)
/home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress_per_unit.py
  /home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress_per_unit.py:320:10 - error: Expression of type "IngressPerUnitProviderEvents" cannot be assigned to declared type "property"
    "IngressPerUnitProviderEvents" is incompatible with "property" (reportGeneralTypeIssues)
  /home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress_per_unit.py:578:16 - error: Expression of type "_RelationEventSnapshot" cannot be assigned to return type "dict[Unknown, Unknown]"
    "_RelationEventSnapshot" is incompatible with "dict[Unknown, Unknown]" (reportGeneralTypeIssues)
  /home/ben/w/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress_per_unit.py:581:25 - error: Argument of type "dict[Unknown, Unknown]" cannot be assigned to parameter "snapshot" of type "_RelationEventSnapshot" in function "restore"
    "dict[Unknown, Unknown]" is incompatible with "_RelationEventSnapshot" (reportGeneralTypeIssues)
7 errors, 0 warnings, 0 informations
@benhoyt benhoyt mentioned this pull request Jan 19, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@rwcarlsen rwcarlsen rwcarlsen approved these changes

@jnsgruk jnsgruk jnsgruk approved these changes

@pengale pengale Awaiting requested review from pengale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PietroPasotti @jnsgruk @rwcarlsen