Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Harness should error on "only simple types" marshalling errors #1115

Closed
sed-i opened this issue Jan 26, 2024 · 2 comments
Closed

Harness should error on "only simple types" marshalling errors #1115

sed-i opened this issue Jan 26, 2024 · 2 comments
Assignees
Labels
small item testing Related to ops.testing

Comments

@sed-i
Copy link
Contributor

sed-i commented Jan 26, 2024

All prometheus unit tests passed with a "complex" stored state in place, but it fails when deployed.
Can harness catch that? (Scenario did.)

unit-prom-0: 22:20:56.564 DEBUG unit.prom/0.juju-log Emitting custom event <CollectStatusEvent via PrometheusCharm/on/collect_unit_status[3]>.
unit-prom-0: 22:20:56.595 ERROR unit.prom/0.juju-log Uncaught exception while in charm code:
Traceback (most recent call last):
  File "./src/charm.py", line 1050, in <module>
    main(PrometheusCharm)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/main.py", line 440, in main
    framework.commit()
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/framework.py", line 694, in commit
    self.on.commit.emit()
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/framework.py", line 351, in emit
    framework._emit(event)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/framework.py", line 853, in _emit
    self._reemit(event_path)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/framework.py", line 942, in _reemit
    custom_handler(event)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/framework.py", line 1050, in on_commit
    self.framework.save_snapshot(self)
  File "/var/lib/juju/agents/unit-prom-0/charm/venv/ops/framework.py", line 728, in save_snapshot
    raise ValueError(msg.format(value.__class__.__name__, data)) from None
ValueError: unable to save the data for StoredStateData, it must contain only simple types: {'status': {'retention_size': ActiveStatus(''), 'k8s_patch': ActiveStatus(''), 'config': ActiveStatus('')}}
@benhoyt
Copy link
Collaborator

benhoyt commented Jan 26, 2024

This is a good idea, thanks. We'll have to figure out where to call save_snapshot in the Harness lifecycle, or otherwise check the values in stored state. Happy to accept a PR, otherwise we'll put it on our backlog.

@benhoyt
Copy link
Collaborator

benhoyt commented Mar 13, 2024

We had a couple of goes at this, but it's a bit tricky to know when to validate. We could do it by calling framework.commit() or emitting framework.on.commit in Harness.cleanup, but we're not sure we want to do that as then charms that observe commit will suddenly start having code run during harness cleanup ... which doesn't seem right. We could put hooks in the non-test Ops code, but that's messy too.

So for now we've decided we'll close this as won't do. If this becomes easy after refactoring for #736 is done, we can reopen it.

@benhoyt benhoyt closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small item testing Related to ops.testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants