feat: ops.testing autoload support for charmcraft extensions#2367
feat: ops.testing autoload support for charmcraft extensions#2367tonyandrewmeyer merged 20 commits intocanonical:mainfrom
Conversation
Use ops.testing (Scenario) with the new charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use ops.testing (Scenario) with the new charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use ops.testing (Scenario) with the new charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use ops.testing (Scenario) with the new charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use ops.testing (Scenario) with the new charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base.
|
@james-garner-canonical , @dimaqq please see #1699 for some thoughts on how the issue could be solved. If you think that the one in this PR isn't the right one, let's have a team chat at daily about what we want to do. I feel like this is the right approach for the moment, and it's not locking us in too much (we could definitely import from charmcraft or some vendored charmcraft code without changing the API, we could definitely automate getting the data without changing the API, if we added a "run charmcraft expand-extensions" system we'd probably want to keep what's here as a fallback, etc). |
james-garner-canonical
left a comment
There was a problem hiding this comment.
I think this is a nice approach to supporting Charmcraft extensions, and I agree with your decision to defer updating automatically until we need to.
I'm marking my review as 'request changes' since I'd like these specific concerns to be addressed before merging:
- On the code generation side, I think we should drop all the manual formatting code since we run
tox -e formatat the end. - On the Scenario side,
_apply_extensionsshould either mutate in place, or return a new dict, but not both. - Should it be an error (or at least a logging error or warning) if the extension defines a field and the charm overrides it with a different type (dict vs list)? How does Charmcraft handle this?
I've made a number of other comments and suggestions as I went, just putting into writing my thoughts as I reviewed. I think they'd improve readability going forward, but if you disagree then don't consider them blocking.
Charmcraft does have some validation that prevents some clashes. I'm not particularly keen on copying that into Scenario, and I think people will run into it at packing time, so it shouldn't really be an issue. If you do something simple like redefine an config option then you get an error (not just in pack, but also e.g. in clean) that says that there are overlapping keys and you need to remove or remove that key. I think it'd be reasonable to make this an error in Scenario as well, even though you're going to quickly hit it elsewhere anyway. |
Co-authored-by: James Garner <james.garner@canonical.com>
Co-authored-by: James Garner <james.garner@canonical.com>
Agreed on not copying that logic. I'm happy with documenting that Scenario won't catch metadata that Charmcraft will consider invalid and/or adding a simple error case -- e.g. should the final |
Co-authored-by: James Garner <james.garner@canonical.com>
|
james-garner-canonical
left a comment
There was a problem hiding this comment.
Looks good!
* Add unit tests for custom charm functionality Use ops.testing (Scenario) with the proposed charmcraft extension autoloading from canonical/operator#2367 to test the charm's custom code on top of the paas-charm base. * chore: bump ops to get the 12-factor improvements in opst[testing] * ci: ops[testing] is required for state transition tests * ci: combine lint and static, per current best practice * ci: run the charm tests as well. * test: remove unnecessary sys.path manipulation * Apply suggestions from code review Co-authored-by: Tony Meyer <tony.meyer@gmail.com>
When using a
charmcraftextension (such as the 12-factor ones), the charm's charmcraft.yaml` has metadata, config, and actions added before being written out to the individual YAML files in the packed charm. Scenario doesn't understand this, so tests fail to make the right events available, and so on.The PR adds a script that extracts the changes that each extension makes by running the
charmcraftCLI tool, and saves that as a series of dictionaries in a private Scenario module. When Scenario autoloads, if there is an extension specified then it starts from the extension data and merges in the local data, simulating what charmcraft does when packing.Changes to the extensions are infrequent, so we can start by manually regenerating the module when needed. If necessary, we could add a workflow that would automate this in the future.
Fixes #1699