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

docs: move Pebble to a separate page #1392

Merged

Conversation

tonyandrewmeyer
Copy link
Contributor

Pros:

  • Significantly reduces the length of the right-hand sidebar for the main API reference
  • Clearly delineates ops.pebble, which could legitimately used independently of ops as a Python Pebble API
  • Separates duplication between ops.Container and ops.pebble into separate pages

Cons:

  • Browser search-on-page is less useful (although we already started this process by splitting ops.testing out)
  • Existing deep links into the docs will break (we should be able to find and fix the ones on juju.is/docs at least)

@tonyandrewmeyer
Copy link
Contributor Author

@tmihoc feel free to provide an opinion here too, if you have time & want to :)

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 26, 2024

@tonyandrewmeyer RTD docs are failing to build. I'll wait to review till you've fixed that so I can compare with https://ops.readthedocs.io/en/latest/

@tonyandrewmeyer
Copy link
Contributor Author

@tonyandrewmeyer RTD docs are failing to build. I'll wait to review till you've fixed that so I can compare with https://ops.readthedocs.io/en/latest/

Huh, they built without error locally, and I didn't get an email about the failure; weird. Thanks for letting me know - I'll figure out what's going on.

@tonyandrewmeyer
Copy link
Contributor Author

Oops, this might explain it 😊

tameyer@tam-canoncial-1:~/code/operator$ git status
On branch pebble-doc-separate-page
Your branch is up to date with 'origin/pebble-doc-separate-page'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	docs/pebble.rst

nothing added to commit but untracked files present (use "git add" to track)

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Sep 26, 2024

@benhoyt it should be ready now, sorry.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

One change requested. Otherwise, I'm happy with this. Let's just follow up with fixing the links in the Juju.is docs.

docs/pebble.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

This change makes sense to me, and looks good when built.

@tonyandrewmeyer tonyandrewmeyer merged commit dcb4d78 into canonical:main Sep 26, 2024
30 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the pebble-doc-separate-page branch September 26, 2024 04:55
Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

👍🏻

@tmihoc
Copy link
Member

tmihoc commented Sep 26, 2024

@tmihoc feel free to provide an opinion here too, if you have time & want to :)

Giving that the Pebble module is indeed quite independent, I think the move makes sense.

Unrelatedly: Looking at https://ops.readthedocs.io/en/latest/index.html, the Navigation, there are two things that I'd do differently:

(1) nitpick: Unit testing (was: Scenario): The "was" there is not great. I'd rephrase as "Unit testing (previously, "Scenario").
(2) a bigger concern: API reference, Pebble client,
Unit testing (was: Scenario),
Harness (legacy unit testing): The titles don't help me see the pieces and the relationships between them clearly. API reference = for what? How does it connect to the Pebble client or the Unit testing or the Harness bits? I don't have a clear suggestion but I think we should take a moment to think about this.

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Sep 26, 2024

(1) nitpick: Unit testing (was: Scenario): The "was" there is not great. I'd rephrase as "Unit testing (previously, "Scenario").

This is intended to only be there for a few months, for what it's worth, to help out all the people that know of Scenario understand that it's the same thing. It could be "previously", although it does then wrap to two lines, which is not ideal.

(2) a bigger concern: API reference, Pebble client, Unit testing (was: Scenario), Harness (legacy unit testing): The titles don't help me see the pieces and the relationships between them clearly. API reference = for what?

It could be "Ops API reference" I guess? If you look down from the top of the page, it says "The Ops Library", then "The ops library documentation" and then "API reference".

How does it connect to the Pebble client or the Unit testing or the Harness bits?

I think Pebble is tricky because ops wraps the Pebble client in the Container object, so for the most part charmers should not need to worry about what's in "Pebble client" because they should be using what's in the "Container" section of "API reference". However, Container leaks a bunch of ops.pebble stuff (error classes, layers, etc) so it's not entirely wrapped, and charmers do need to go to "Pebble client" for those (and "client" in that case is somewhat wrong).

We could introduce a layer, like:

  • Development
    • API reference
    • Pebble
  • Testing
    • Unit testing (previously: Scenario)
    • Harness (legacy unit testing)

Would that help?

It could be by namespace, but we decided to merge the two APIs in ops.testing, so that would be:

  • ops
  • ops.pebble
  • ops.testing
  • ops.testing (legacy API)

Or something like that.

I don't have a clear suggestion but I think we should take a moment to think about this.

Sure :)

@tmihoc
Copy link
Member

tmihoc commented Sep 26, 2024

It could be by namespace, but we decided to merge the two APIs in ops.testing, so that would be:

  • ops
  • ops.pebble
  • ops.testing
  • ops.testing (legacy API)

This makes the most sense to me and I just typed something like this too on your other PR: #1320 (comment)

With a small difference:

  • ops (natural order:)
    • ops.main entrypoint
    • ops.main (alphabetical order:)
      • ActionEvent
      • ...
    • ops.pebble
    • ops.testing
    • ops.testing.harness (legacy)

Where ops is basically the top-most layer and may not need a separate entry in Navigation -- so the Navigation can skip to the component items. I wouldn't bother to group them into Development and Testing -- the extra layers imo bury content rather than helping surface it. We could group them like that in the top-level intro, if at all. E.g.,

The ops library provides:

  • development tools:
    • ops.main entrypoint
    • ops.main
    • ops.pebble
  • testing tools:
    • ...

tonyandrewmeyer added a commit to tonyandrewmeyer/operator that referenced this pull request Oct 4, 2024
Pros:
* Significantly reduces the length of the right-hand sidebar for the
main API reference
* Clearly delineates ops.pebble, which could legitimately used
independently of ops as a Python Pebble API
* Separates duplication between ops.Container and ops.pebble into
separate pages

Cons:
* Browser search-on-page is less useful (although we already started
this process by splitting ops.testing out)
* Existing deep links into the docs will break (we should be able to
find and fix the ones on juju.is/docs at least)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants