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: update 12-factor tutorials #2085

Merged
merged 117 commits into from
Feb 19, 2025
Merged

Conversation

erinecon
Copy link
Contributor

Update the 12-factor tutorials.

Specific updates:

  • Convert to reStructuredText
  • Incorporate feedback from UX session with Daniele
  • Update all tutorials for consistency (feedback from previous PRs, feedback from Daniele)

@lengau
Copy link
Collaborator

lengau commented Jan 18, 2025

Hey Erin, I really appreciate this work! Just a note, this is going to conflict with #2083 as it also moves these files to rst. You may want to try to coordinate with @jahn-junior, though I imagine we'll be merging his PR on Monday or Tuesday.

@erinecon
Copy link
Contributor Author

Hey Erin, I really appreciate this work! Just a note, this is going to conflict with #2083 as it also moves these files to rst. You may want to try to coordinate with @jahn-junior, though I imagine we'll be merging his PR on Monday or Tuesday.

Hi @lengau, thanks for letting me know about the other PR. I was under the impression that the 12-factor docs weren't going to be touched since we had planned work to refactor them (See this comment on PR #2010 ). I see there's also #2048 with the how-to guide that I'm planning on refactoring, but that's for a later PR, so I will hold off on that work until after the PR has been merged.

It seems unfortunately that any changes to the Django tutorial might be overwritten by the work in this PR, but I will not touch the FastAPI or Go tutorials until after #2083 has been merged. Thanks again for letting me know.

@lengau
Copy link
Collaborator

lengau commented Jan 21, 2025

Hey @erinecon !

On #2048: I'm happy for you to take over that one or to close it unmerged if you want to redo the MD -> rST conversion in your own PR, or we can work together to prioritise the completion so it won't affect you.

Hopefully you should be able to rebase this PR either on #2083 or on main once it's merged and not have too many conflicts.

@erinecon
Copy link
Contributor Author

Hi @lengau , thanks for the offer. I'm fine with waiting for #2048 to be merged before working on a new PR. I appreciate the work you're all doing to convert the files to rST, it saves me a bunch of time! :)

@erinecon erinecon changed the title update 12-factor tutorials docs: update 12-factor tutorials Jan 21, 2025
@lengau lengau self-requested a review February 14, 2025 18:51
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +11 to +13
.. code-block:: bash

multipass launch --cpus 4 --disk 50G --memory 4G --name charm-dev 24.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed this, which is probably not how you intend it to look:

image
Changing to:

Suggested change
.. code-block:: bash
multipass launch --cpus 4 --disk 50G --memory 4G --name charm-dev 24.04
.. code-block:: terminal
multipass launch --cpus 4 --disk 50G --memory 4G --name charm-dev 24.04

changes the highlighting (this is from a different branch which is why the text is different):

image

Copy link
Contributor

@medubelko medubelko Feb 14, 2025

Choose a reason for hiding this comment

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

terminal falls back to no highlighting, as it's not a Pygments lexer:

WARNING: Pygments lexer name 'terminal' is not known

Other tools that use Pygments know how to read it correctly. An example would be VS Code:

image

I'm blaming our doc stack. If the issue is really bothersome we can set the lexer to text for those odd lines. It's not highlighting much, anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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


.. note::

The ``psycopg2-binary`` package is needed so the Flask application can
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand why the tutorial is this way, but at some point it'd probably be worth documenting uhh... somewhere (no idea where is the right place except that it's definitely out of scope here) how to do the "preferred" method of getting psycopg2 from source and including relevant Ubuntu packages in the rock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Thanks for all this work Erin! Everything else is well beyond me so I'm approving for whatever you and @medubelko do! If one of you will ping me when it's ready to merge I can add it to the queue. I think Michael can too, and if not I should fix that.

Copy link
Contributor

@medubelko medubelko left a comment

Choose a reason for hiding this comment

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

Lots of little bits to consider for the Django tutorial.

Use that to create a charm with ``charmcraft``. Use that to test-deploy, configure,
etc., your Django application on a local Kubernetes cloud, ``microk8s``, with
``juju``. All of that multiple times, mimicking a real development process.
This tutorial should take 90 minutes for you to complete.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch. :( If they take this long, we should revisit and split them later in another cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Django probably does take closer to 90 min, but the other frameworks probably take closer to 60 minutes with a decently powerful machine and stable internet. The UX research showed that 90 minutes is closer to reality -- and somewhat surprisingly, the research also showed that the majority of users didn't mind the length of any of the tutorials. They appreciated that the tutorial was thorough and stepped through the entire production process.

We can split them and test with users to see whether it harms the UX -- something to consider down the line

Comment on lines 19 to 24
If you're new to the charming world: Django applications are
specifically supported with a coordinated pair of profiles
for an OCI container image (**rock**) and corresponding
packaged software (**charm**) that allow for the application
to be deployed, integrated and operated on a Kubernetes
cluster with the Juju orchestration engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

In discussion offline.


A package consisting of YAML files + Python code that will automate every
aspect of an application's lifecycle so it can be easily orchestrated with Juju.
- A working station, e.g., a laptop, with amd64 or arm64 architecture
Copy link
Contributor

Choose a reason for hiding this comment

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

amd64

There are several minute microcopy/terminology patterns, some in the style guide and some without, that we'd like to hold Starcraft docs to. Would you mind if I committed them to your branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's fine with me :)


.. code-block:: bash
.. note::
Copy link
Contributor

Choose a reason for hiding this comment

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

The text sounds essential, not optional.

for example:
Now you can open a new tab and visit http://django-hello-world. The
Django app should respond in the browser with
``The install worked successfully! Congratulations!``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Block quote?


from django.contrib import admin
from django.urls import include, path
The generated Django application does not come with an app, which is why
Copy link
Contributor

Choose a reason for hiding this comment

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

application does not come with an app

Potentially confusing...

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 agree with you, but AFAIK, this is the terminology used by Django devs. Originally we called this section "Add a root endpoint", but a certain tester said that this would be unknown and confusing to the average Django developer.

Comment on lines 548 to 549
``django.urls`` to contain ``include`` and the value of ``urlpatterns`` to
include ``path('', include("greeting.urls")`` like in the following example:
Copy link
Contributor

Choose a reason for hiding this comment

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

include... to include

A bit difficult to parse. Simplify?

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 gave it an attempt, please let me know if you think it's clearer

Copy link
Contributor

@evildmp evildmp left a comment

Choose a reason for hiding this comment

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

This looks excellent, thanks for all the work that has gone into creating and reviewing it.

Let's get it merged now and published and improve in the next iteration. I'd like to see it up as soon as possible.

@@ -0,0 +1,93 @@
First, install Multipass.

.. seealso::
Copy link
Contributor

Choose a reason for hiding this comment

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

We use blockquotes for these in the rest of the docs.

We should use blockquotes for quoting nlocks of text and nothing else. The Juju CTA examples are incorrect and should be changed. seealso is the correct way to do it.

@lengau lengau merged commit 79fa5ad into canonical:main Feb 19, 2025
30 of 31 checks passed
dwilding added a commit to canonical/operator that referenced this pull request Feb 19, 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**

- The link in
https://ops.readthedocs.io/en/latest/howto/get-started-with-charm-testing.html#charmcraft-profiles
that should point to the Charmcraft docs. I'll follow up with a separate
PR after canonical/charmcraft#2085 gets merged
into Charmcraft.

- Links in the code samples of the Kubernetes tutorial (for example, in
[Create a minimal Kubernetes
charm](https://ops.readthedocs.io/en/latest/tutorial/from-zero-to-hero-write-your-first-kubernetes-charm/create-a-minimal-kubernetes-charm.html)).
I think it would be better to tackle these separately when improving the
tutorial - @tonyandrewmeyer what do you think?
@erinecon erinecon deleted the update-tutorials branch February 19, 2025 23:53
@erinecon erinecon restored the update-tutorials branch February 24, 2025 19:40
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