feat: add support for cloud spec

[IronCore864]

Add support for cloud spec with an e2e test.

[PietroPasotti]

Nice work!
@tonyandrewmeyer let me know what you think about the cloudspec dataclass ownership, we should make that decision once and for all if we didn't already :)

[IronCore864]

Thanks for the review, I have refactored according to your comments. Please take another look :)

[PietroPasotti]

Looks better! Thanks :)
One more round and I think we're there

[tonyandrewmeyer]

Added a few comments as well.

One other thing: I think it would be worth having the consistency checker validate that State.cloud_spec is None if State.model.type == "kubernetes"

[IronCore864]

After discussion in standup, we decided to keep the CloudSpec dataclass in scenario but remove the from_dict method for now.

[benhoyt]

Adding as a top-level comment as well to avoid it getting lost: it seems strange to me to have AuthType as an enum but not cloud type. I think we should do none or both, unless there's a good reason not to? It looks like we're leaning towards enums, so let's do enums for both. We should also link to the Juju source in a comment, for reference.

[IronCore864]

After discussion in the standup today we decided to use str, instead of Enum at some places then str at other places. Also it's consistent with ops. Closing some comments due to this.

[IronCore864]

I have done yet another refactoring according to your reviews, please take a look @tonyandrewmeyer @PietroPasotti.

For the record, I still don't feel this is right, but I've done it anyway to try to make it consistent:

• copy-pasting ops.CloudSpec and ops.CloudCredential to scenario
• delete from_dict method, which is what's used in ops, and is (subjectively) easier to use than instantiating a class
• add a conversion method so that it returns an ops object (it's not like that by porting the ops.CloudSpec to scenario, it is separated with ops and has nothing to do with ops any more)
• build two objects in the UT which are the same.

For another record, there are inconsistent code in consistency_checker (the irony is in the file name 🤣):

Some places do:

errors = []
warnings = []

# do something

return Results(errors, warnings)

Other places do:

errors = []

# do something

return Results(errors, [])

I think the latter is less readable since I need to click into the definition of Results to know what the second parameter is and why it's an empty list. In this PR I chose the first one; I will create another PR to fix the second cases to the first so that the code is more consistent.

[tonyandrewmeyer]

Would you be able to add a short example into the README? Ideally, we have all of the state documented there.

[IronCore864]

Would you be able to add a short example into the README? Ideally, we have all of the state documented there.

README updated.

Also made "to ops" methods "private". It makes sense to make them private; I thought one of them was used in the test but it wasn't, my bad.

Only discussion left open: _to_ops, _to_ops_object, _to_ops_cloud_spec naming convention.

[PietroPasotti]

Would you be able to add a short example into the README? Ideally, we have all of the state documented there.

README updated.

Also made "to ops" methods "private". It makes sense to make them private; I thought one of them was used in the test but it wasn't, my bad.

Only discussion left open: _to_ops, _to_ops_object, _to_ops_cloud_spec naming convention.

IMHO _to_ops is clear enough, so long as there's only one ops object that each scenario.state one can map to.

[benhoyt]

Very close -- just a few nit comments now.

[IronCore864]

Very close -- just a few nit comments now.

All updated, please take another look.

[benhoyt]

Great, thanks for the updates!

[tonyandrewmeyer]

Looks good to me - thanks! I would suggest that @PietroPasotti has a final review before it's merged, though, since I'm still pretty new to Scenario.

[PietroPasotti]

Some thoughts on the defaults that need discussing, but the rest looks good! 👍

[IronCore864]

@PietroPasotti can't reply you in one of your comment so I'm putting it here in another separate comment: Regarding enum vs str, there is a discussion in previous comments where we decided to make it consistent as str, so it's resolved.

[PietroPasotti]

@IronCore864 @tonyandrewmeyer where are we on this PR?

[tonyandrewmeyer]

@IronCore864 @tonyandrewmeyer where are we on this PR?

I thought everything had been resolved (merging is fine with me). @IronCore864 were you wanting a final review from @PietroPasotti ?

[IronCore864]

Sorry for getting back at it so late, I just finished some other tasks and could only get back at it now.

It seems all comments are resolved, and in my latest commit, I fixed the comment improvement specified by @PietroPasotti.

@tonyandrewmeyer it's ready to be merged, I don't have write access to this repo.