Conversation
kaustavb12
left a comment
There was a problem hiding this comment.
Left a few comments, but otherwise LGTM 👍
👍
- I tested this: Setup mkdocs in local and read through docs
- I read through the code
- I checked for accessibility issues
- Includes documentation
- Added to the Code Drift project board (for backports)
|
One thing I found missing in the new docs was instructions on setting up a cluster repo in github. These can be confusing for a first time user, specially things like setting up actions secrets. Also, we should briefly talk about the different workflows and how to trigger them, plus what are some of the workflow common inputs, such as "strain", "runner label", "phd cli version", etc. |
Agrendalath
left a comment
There was a problem hiding this comment.
Thank you for preparing such detailed documentation, @gabor-boros! I left some suggestions.
| uv pip install -r docs/requirements.txt | ||
| ``` | ||
|
|
||
| This creates a virtual environment (`.venv`) at the repository root and installs the MkDocs dependencies, making them available to `uv run` commands. |
There was a problem hiding this comment.
Nit: if uv is already a prerequisite, we can simply run uvx --with mkdocs-material --with mkdocs-glightbox mkdocs. It does not require any virtualenv or dependency management.
It would be a one-liner in the CI. If needed, we could also pin mkdocs version to avoid automatically upgrading it to 2.0.
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v4 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: 3.12 |
There was a problem hiding this comment.
| - name: Install uv | |
| uses: astral-sh/setup-uv@v4 | |
| - uses: actions/setup-python@v5 | |
| with: | |
| python-version: 3.12 | |
| - name: Install uv and set the python version | |
| uses: astral-sh/setup-uv@v4 | |
| with: | |
| python-version: 3.12 |
Nit - uv action already sets up Python.
| - name: Set cache ID | ||
| run: echo "cache_id=$(date --utc '+%V')" >> $GITHUB_ENV | ||
| - uses: actions/cache@v4 | ||
| with: | ||
| key: mkdocs-material-${{ env.cache_id }} | ||
| path: ~/.cache | ||
| restore-keys: | | ||
| mkdocs-material- |
There was a problem hiding this comment.
| - name: Set cache ID | |
| run: echo "cache_id=$(date --utc '+%V')" >> $GITHUB_ENV | |
| - uses: actions/cache@v4 | |
| with: | |
| key: mkdocs-material-${{ env.cache_id }} | |
| path: ~/.cache | |
| restore-keys: | | |
| mkdocs-material- |
Nit: with these dependencies, running the cache action will take more time than installing everything from scratch with uv.
| @@ -0,0 +1,93 @@ | |||
| # Documentation | |||
|
|
|||
| This directory contains the documentation for the Launchpad (Launchpad) cluster template, built with [MkDocs](https://www.mkdocs.org/) and the [Material theme](https://squidfunk.github.io/mkdocs-material/). | |||
There was a problem hiding this comment.
Are we renaming PHD to Launchpad? Isn't it too close to the name of the website owned by Canonical?
| - **Database Snapshots**: Some cloud providers maintain database snapshots | ||
| - **Storage Buckets**: If buckets weren't destroyed, data may still be accessible |
There was a problem hiding this comment.
If these are managed by Terraform and not marked with prevent_destroy, they will be deleted. We should probably add a huge red warning at the top of this page.
There was a problem hiding this comment.
While I see where are you coming from for this, I believe "infrastructure de-provisioning" and "destroy" are pretty self-explanatory for indicating that something irreversible will happen. also, the descriptions are explicitly telling that resources will be destroyed.
|
|
||
| Open edX services write logs to stdout. Log level and format are controlled by application settings (e.g. Django `LOGGING`, Celery, or environment variables). Adjust these in the instance configuration or in custom settings if you need different verbosity or structure. | ||
|
|
||
| To get the application logs, use: |
There was a problem hiding this comment.
Can we browse all logs from a central place? IIRC, Harmony provisioned an OpenSearch instance.
There was a problem hiding this comment.
It depends on Harmony. However, setting that up should not be part of this documentation for the reasons mentioned above; rather, if we feel the need for it, let's update the documentation of Harmony. I wouldn't like to blow up the already monumental scope. Would you be up for opening a task for this under the corresponding epic?
| - **Tutor** generates the Kubernetes YAML for the instance (Deployments, Services, Ingress, etc.) from `config.yml`. | ||
| - **Drydock** is a Tutor plugin used in this stack; options such as `DRYDOCK_INIT_JOBS` and `DRYDOCK_REGISTRY_CREDENTIALS` are set in `config.yml`. | ||
|
|
||
| Regenerate manifests with Tutor from the instance config and commit the result to the cluster repo so ArgoCD can sync. |
There was a problem hiding this comment.
Is there a command for this?
There was a problem hiding this comment.
The pipelines are doing this, this is just an explanation what will happen. By the way, it is done by tutor config save IIRC. An operator will have nothing to do with this besides kicking off the pipeline.
| ## Overview | ||
|
|
||
| - **Picasso** provides GitHub Actions workflows to build Docker images from the instance configuration and Tutor/Picasso setup. | ||
| - Image names and tags are typically set in the instance `config.yml` (e.g. `DOCKER_IMAGE_OPENEDX`, `MFE_DOCKER_IMAGE`) and automatically updated by Picasso after a build. |
There was a problem hiding this comment.
An example config would be nice here.
SE-6524 Signed-off-by: Gabor Boros <gabor@opencraft.com>
SE-6525 Signed-off-by: Gabor Boros <gabor@opencraft.com>
67c2a1f to
3d16dfa
Compare
SE-6525 Signed-off-by: Gabor Boros <gabor@opencraft.com>
9e929ed to
ba44b4b
Compare
kaustavb12
left a comment
There was a problem hiding this comment.
LGTM 👍
- I tested this: Spun up mkdocs in local and read through the docs
- I read through the code
SE-6524 Co-authored-by: Kaustav Banerjee <kaustav@opencraft.com>
|
The rest of the comments can be resolved in a follow-up PR (or multiple ones). Merging now. |
[SE-6524]