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: fix links that pointed to earlier Juju docs #1575

Merged
merged 13 commits into from
Feb 19, 2025

Conversation

dwilding
Copy link
Contributor

@dwilding dwilding commented Feb 12, 2025

This PR replaces links to https://juju.is/docs/ by internal cross-references to other Ops pages or intersphinx cross-references to Juju pages (as appropriate).

Not included in this PR

@@ -136,7 +136,6 @@ It is in our interest to move the handler logic for each `/hooks/<hook_name>` to
- We can avoid code duplication by accessing shared data via the CharmBase interface provided through `self`.
- The code is all in one place, easier to maintain.
- We automatically have one Python object we can test, instead of going back and forth between Bash scripts and Python wrappers.
- We can use [the awesome testing Harness](https://juju.is/docs/sdk/testing).
Copy link
Contributor Author

@dwilding dwilding Feb 12, 2025

Choose a reason for hiding this comment

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

My reasoning for this change: The preceding bullet already mentions testing. And Harness is not considered so awesome any more 💔

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to remove, but you could swap out Scenario for Harness (except you'd need to say "unit testing framework" or some other generic name) and it would still be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's best to omit the link here, as it's not something we want the reader to start working through right away. I'm adding a small section towards the end of the doc to remind the reader that they should write tests, with a link to Get started with charm testing.

Juju automatically picks up logs from charm code that uses the Python [logging facility](https://docs.python.org/3/library/logging.html), so we can use the Juju [`debug-log` command](https://juju.is/docs/juju/juju-debug-log) to display logs for a model. Note that it shows logs from the charm code (charm container), but not the workload container. Read ["Use `juju debug-log`"](https://juju.is/docs/sdk/get-logs-from-a-kubernetes-charm#heading--use-juju-debug-log) for more information.
Juju automatically picks up logs from charm code that uses the Python [logging facility](https://docs.python.org/3/library/logging.html), so we can use the Juju [`debug-log` command](inv:juju:std:label#command-juju-debug-log) to display logs for a model. Note that it shows logs from the charm code (charm container), but not the workload container.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the link to https://juju.is/docs/sdk/get-logs-from-a-kubernetes-charm#heading--use-juju-debug-log because I can't find where this info has ended up. @tmihoc do you know whether we still have it somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's the same content, but I think we should be linking here: https://canonical-juju.readthedocs-hosted.com/en/latest/user/howto/manage-logs/

Also: we should make this a See also: rather than having it in the paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think Juju | How to manage logs addresses the point in such an explicit way. Anyway, I agree it's a more appropriate place to link to. Good idea about "see also" - I'm putting it in a "see more", to be consistent with the rest of the page.

Copy link
Member

Choose a reason for hiding this comment

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

I'd use a `>See first: Juju | Manage logs'. I've done that for both Charmcraft and Ops. The idea being that we want people to be aware of what a thing looks like on the charm user end first, as that's likely to be the fastest way for them to catch up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I just realised that we already have a "see first" link to that page. I've updated the "see first" block to list the how-to link before the reference link.

Copy link
Contributor Author

@dwilding dwilding Feb 19, 2025

Choose a reason for hiding this comment

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

Oops, I just found the commit for this sitting in my editor. I thought I'd pushed it, but apparently not... I'll do a separate PR for it: #1584

@dwilding dwilding marked this pull request as ready for review February 14, 2025 00:14
Juju automatically picks up logs from charm code that uses the Python [logging facility](https://docs.python.org/3/library/logging.html), so we can use the Juju [`debug-log` command](https://juju.is/docs/juju/juju-debug-log) to display logs for a model. Note that it shows logs from the charm code (charm container), but not the workload container. Read ["Use `juju debug-log`"](https://juju.is/docs/sdk/get-logs-from-a-kubernetes-charm#heading--use-juju-debug-log) for more information.
Juju automatically picks up logs from charm code that uses the Python [logging facility](https://docs.python.org/3/library/logging.html), so we can use the Juju [`debug-log` command](inv:juju:std:label#command-juju-debug-log) to display logs for a model. Note that it shows logs from the charm code (charm container), but not the workload container.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's the same content, but I think we should be linking here: https://canonical-juju.readthedocs-hosted.com/en/latest/user/howto/manage-logs/

Also: we should make this a See also: rather than having it in the paragraph.

@@ -136,7 +136,6 @@ It is in our interest to move the handler logic for each `/hooks/<hook_name>` to
- We can avoid code duplication by accessing shared data via the CharmBase interface provided through `self`.
- The code is all in one place, easier to maintain.
- We automatically have one Python object we can test, instead of going back and forth between Bash scripts and Python wrappers.
- We can use [the awesome testing Harness](https://juju.is/docs/sdk/testing).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to remove, but you could swap out Scenario for Harness (except you'd need to say "unit testing framework" or some other generic name) and it would still be true.

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.

Look good! I built the docs locally and went through to check that the links all worked. Approving as I don't really have anything significant to add over Tony's review.

@tonyandrewmeyer
Copy link
Contributor

I built the docs locally and went through to check that the links all worked.

I did the same with the preview build on rtd, by the way. So between us we have good coverage :)

@james-garner-canonical
Copy link
Contributor

I built the docs locally and went through to check that the links all worked.

I did the same with the preview build on rtd, by the way. So between us we have good coverage :)

There's a preview build!?

@dwilding
Copy link
Contributor Author

There's a preview build!?

The preview build is here: https://ops--1575.org.readthedocs.build/en/1575/. You can find it by clicking Details for the check called "docs/readthedocs.org:ops". Sorry, I should have pasted it into the PR description (I usually do that).

@dwilding
Copy link
Contributor Author

Thanks everyone for your feedback! @tonyandrewmeyer I've addressed all the comments now - please could you review again when you get a chance?

One thing to note: while updating the links to Set things up and Tear things down, I also made some formatting changes to smooth out the flow (to my eyes). These changes: 5f4828a

Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

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

Thanks - looking great!

@dwilding dwilding merged commit 0e565da into canonical:main Feb 19, 2025
32 checks passed
@dwilding dwilding deleted the fix-external-links branch February 19, 2025 23:15
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.

4 participants