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

Add minimum juju version to event docstring #1305

Closed
sed-i opened this issue Jul 31, 2024 · 7 comments · Fixed by #1311
Closed

Add minimum juju version to event docstring #1305

sed-i opened this issue Jul 31, 2024 · 7 comments · Fixed by #1311
Labels
docs Improvements or additions to documentation

Comments

@sed-i
Copy link
Contributor

sed-i commented Jul 31, 2024

Some events are only available from a certain version of juju.

class PebbleCheckFailedEvent(PebbleCheckEvent):

It would be handy to have it documented in the docstring.

@tonyandrewmeyer tonyandrewmeyer added the docs Improvements or additions to documentation label Aug 1, 2024
@tonyandrewmeyer
Copy link
Contributor

tonyandrewmeyer commented Aug 1, 2024

For what it's worth, those events do have the required version documented in Juju's hook list, and in the how to guide.

I do think these should be documented (e.g. the hook list has the 3.6 and 4.0 changes, but doesn't say when secret events became available). I'm not sure whether ops API reference docs are the place, or whether we should be doing more to link to other docs. This is at least information that shouldn't need to change. We have a few existing notes but they are vague "older versions of Juju", and I assume there must have been some reason for that.

For the events specifically, I know @tmihoc has plans for restructuring how these are documented. @tmihoc do you have existing plans on where "available from Juju version x" information should go? Would you just include it everywhere since it's fairly small and shouldn't need updating?

@dimaqq
Copy link
Contributor

dimaqq commented Aug 1, 2024

+1 from me on including juju version in the doctring specifically.

that would help folks who use IDE's and may have to support a wider range of Juju versions.

@tmihoc
Copy link
Member

tmihoc commented Aug 14, 2024

This is a bit tricky: The fact that some events are only available starting with a certain version of Juju is, of course, because Ops events piggyback on Juju hooks, so they will only be available starting with the version of Juju where those hooks are defined. In a way, putting this information on the events is misleading. Imo this should rather be document in the hook docs and the event reference should link to those, as I've mentioned. Unfortunately the hook docs are still WIP. However, there have been developments. Let me get back to you on this in a couple of weeks.

@tonyandrewmeyer
Copy link
Contributor

Imo this should rather be document in the hook docs and the event reference should link to those, as I've mentioned.

I did suggest that to the team. The push back was:

  • If it's something specific like "To find the versions of Juju this is available in see LINK", then forcing people to go through the link feels pretty hostile, particularly for information that won't ever change so isn't at risk of being out of sync.
  • If it's more generic like "See more: Start hook LINK", then this is probably good to have for all the event classes, and/or all the CharmEvents attributes, once the hook docs improvements are done. However, it's not immediately obvious that it's where you go to find out why you're never getting the event triggered (if the answer is your Juju is too old).

There's a draft version of the docs with this if you haven't seen that and would like to take a look.

@tmihoc
Copy link
Member

tmihoc commented Aug 21, 2024

  • If it's more generic like "See more: Start hook LINK", then this is probably good to have for all the event classes, and/or all the CharmEvents attributes, once the hook docs improvements are done. However, it's not immediately obvious that it's where you go to find out why you're never getting the event triggered (if the answer is your Juju is too old).

That is what I had in mind.

Imo commenting on Juju versions in Ops docs is not a good pattern. Ops docs should comment on which version of Ops something was introduced. Because of the nature of Ops, that will of course also mean a certain version of Juju. Ideally, Juju and Ops and ... should be aligned on versions so that we can speak of an ecosystem version. Short of that, each tool's reference should be versioned (that should take care of the question "which feature is available when") and the relation between the tools should be made transparent (HookEvents should link back to Juju hooks, CharmEvents attributes should link to JuJu hook tools; that should take care of "in which version of Juju do you get hook x and therefore hookevent x"). In-line notes may feel like the lean pragmatic solution but it's a solution that takes shortcuts and can hurt down the line. So, I'm fine with it for now but I don't think it's the right way to go in the long term.)

@tonyandrewmeyer
Copy link
Contributor

That is what I had in mind.

As we discussed earlier in the week, I think it would be quite reasonable to take another look at this once the hook tool doc improvements are done, and consider linking up either in addition to or instead of what we have now.

Ops docs should comment on which version of Ops something was introduced. Because of the nature of Ops, that will of course also mean a certain version of Juju.

That's not the case. You could be using ops 2.15 with Juju 2.9 or ops 2.0 with Juju 4. We don't tie ops releases to Juju releases (pylibjuju has a messy version of that where the version numbers are aligned but the actual functionality available is figured out during the connection to Juju; pebble is bundled with Juju so essentially does have "version x of Juju means version y of Pebble").

For example, you can, and should, use the latest version of ops with your charm that supports Juju 2.9 - but you can't use the ops features that model Juju features that arrived later. You can observe secret events but none will ever arrive because Juju won't send them; if you call unit.set_ports() on a K8s charm then that will crash at runtime because it's only supported in Juju 3.1+.

Ideally, Juju and Ops and ... should be aligned on versions so that we can speak of an ecosystem version.

This isn't the current approach. The current approach is charmers should always use the most recent version of ops, whichever versions of Juju they want their charm to run on. There are quite a few advantages of this over tying a Juju version to an ops version - for example, say your charm supports running on Juju 3.5 and 3.6 - you shouldn't need to pack it twice with different requirements files.

Short of that, each tool's reference should be versioned

I believe at the moment the intention is that major versions of ops are versioned (but we don't have 1.x docs, and there's no 3.x yet so that means just 2.x right now). I don't have an objection to versioning minor versions as well, but there are objections from other Charm-Tech team members, I believe. I don't think bugfix releases should have separate versioned docs.

and the relation between the tools should be made transparent (HookEvents should link back to Juju hooks, CharmEvents attributes should link to JuJu hook tools; that should take care of "in which version of Juju do you get hook x and therefore hookevent x").

I'm generally in favour of these links ... once the content to link to is of higher quality 😄.

If there's something that can change, like the arguments or output of a hook tool, or the description of a hook, etc, then I agree that's best in one place in the Juju docs, and the ops reference links to those. I feel version added is a little different in that it can never change and so never get out of sync, and it's also a very small bit of information. When the upstream content is ready for ops to link to it, I'd like to see us try out leaving the version in and removing it and seeing which seems best.

@tmihoc
Copy link
Member

tmihoc commented Aug 22, 2024

@tonyandrewmeyer Thanks, I appreciate the back and forth!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants