[DPE-7949] Refresh v3 documentation #1087
Conversation
carlcsaposs-canonical
force-pushed
the
refresh-docs-16
branch
from
c811462 to
256aceb
Compare
docs/how-to/refresh.md
Outdated
|
|
||
| ### Supported refreshes | ||
|
|
||
| These refreshes should be supported. |
There was a problem hiding this comment.
I'd prefer to write something less vague, like "These versions are expected to work, but are not officially tested or guaranteed".
There was a problem hiding this comment.
I think I need more clarity from @taurus-forever to make this less vague
My guess is that we're expected to test that these refreshes work (and if they don't work, we would not include them here). And they should be "officially supported"; my understanding is we cannot require intermediate charms within a track
And the recommended refreshes would be different by being much better tested (both by us, and because more users go down that path) and would be less risky (since it's easier to ensure compatibility from a development/code perspective)
But that's what I think is expected
What we're actually delivering, in practice, for now might be different. I would categorize the "supported refreshes" that will be in this section in the next 1-2 years as "we aren't aware of any incompatibilities yet; so we think there's a decent chance it will work"
There was a problem hiding this comment.
Until we get more clarity, I'd prefer to remove this line altogether than use the word "should".
a-velasco
left a comment
a-velasco
left a comment
There was a problem hiding this comment.
Thank you for your work here, @carlcsaposs-canonical. After reading through the whole process carefully, I think your changes to the structure and sequence makes a lot of sense.
Most of my comments are nitpicks and small things. The "big" thing left to do for this PR imo is just converting the file to .rst so that the tables and links are more manageable.
Is this worth it? there's only one column with links so imo it's not too bad as-is |
|
@carlcsaposs-canonical Definitely worth it imo, especially since that list is going to grow. I would not consider it a priority though, and definitely not a blocker for merging this PR. I'll convert it together with another page that needs rST syntax in a different PR. |
reneradoi
left a comment
reneradoi
left a comment
There was a problem hiding this comment.
Nice and extensive docs!
A general suggestions I'd like to add is that I'd like to see this whole document divided into two parts: "preparation" and "during execution" (would start at "Pre-refresh check). This would make it easier for users to digest the long list of instructions and for us to group all of them a bit more logically.
Other than that, I only have some questions and a few comments to details.
docs/how-to/refresh.md
Outdated
|
|
||
| In the event that something goes wrong (e.g. the refresh fails, the new version of PostgreSQL is not performant enough, a database client is incompatible with the new version), you may want to quickly roll back. | ||
|
|
||
| Prepare for this possibility by reading through the entire refresh documentation—with special attention to the [rollback section](#roll-back)—before starting the refresh. |
There was a problem hiding this comment.
Let's put spaces around the dashes - better for readability and line breaks.
This applies for all the document.
There was a problem hiding this comment.
Typically, em dashes are used without spaces (and when rendered for display [i.e. not in a monospace font] are designed for readability without spaces)
Regarding line breaks, not sure how markdown/myst handles it but other tools automatically handle line breaks (replace em dash with em dash and zero-width space), e.g. https://docs.asciidoctor.org/asciidoc/latest/subs/replacements/
There was a problem hiding this comment.
Carl's right about the way the dashes are rendered in the HTML (i.e. as an em-dash, which are much more readable)
That being said, I think it would significantly increase the readability (and grammatical accuracy) of the markdown to use double dashes (indicating em-dash usage) instead of single dashes.
...entire refresh documentation--with special attention...
This also gets rendered correctly as an em-dash.
There was a problem hiding this comment.
grammatical accuracy
fyi it is an em dash in the markdown source (it's just rendered kinda weird by the monospace font)
a—a # em dash
a-a # hyphen
a–a # en dashdouble dash
how does this work & are there any limitations?
it doesn't appear to be native markdown, so don't know how portable this is (e.g. won't render correctly in IDE preview)
a--aa--a
with asciidoc for example, it has to be surrounded by word characters for the replacement to work (e.g. so command line flags don't get replaced) and other syntax has to be used in other cases (e.g. https://github.com/canonical/charm-refresh/blob/0062802c388c2666067851e356fc3c7cd6d941c4/docs/modules/ROOT/pages/user-experience/status.adoc?plain=1#L200)
There was a problem hiding this comment.
I tried -- and it looks like it's getting replaced with an en dash, not an em dash
There was a problem hiding this comment.
Oh wow I didn't realize how subtle the difference was between em and en-dashes in monospace, you're right. Both look super short. Same subtlety in the rendered output, except they both look similarly long.
Two consecutive dashes (--) does not, in fact, render as an em-dash.
Unfortunately there seems to be no good solution to make it more readable in monospace short of changes to your editor's font locally.
TLDR: Em-dash usage should remain as @carlcsaposs-canonical wrote originally, as it's what Sphinx/MyST expects.
There was a problem hiding this comment.
Two consecutive dashes (--) does not, in fact, render as an em-dash.
I think this is a bug
I'm worried that this will cause users to skip the preparation also the separation between preparation and execution is not completely black and white—for example, is creating a backup preparation or execution? same with scaling up, |
paulomach
left a comment
paulomach
left a comment
There was a problem hiding this comment.
Imo quite complete and extensive on the edge of being too much.
For now, I am of the opinion that the best path forward is to publish it, battle test it, and gather feedback, either passive or proactive one from users we have relationship (e.g. mansol, is, support).
reneradoi
left a comment
reneradoi
left a comment
There was a problem hiding this comment.
I agree, this guide is good to go.
a-velasco
left a comment
a-velasco
left a comment
There was a problem hiding this comment.
Just one tiny change, and then imo it is ready to be merged.
docs/how-to/refresh.md
Outdated
|
|
||
| ### Supported refreshes | ||
|
|
||
| These refreshes should be supported. |
There was a problem hiding this comment.
Until we get more clarity, I'd prefer to remove this line altogether than use the word "should".
## Description of issue or feature: The PR adds user documentation for upgrading charmed etcd. ## Solution: The docs are based on the template created in Postgres ([#1087](canonical/postgresql-operator#1087)). ## How was this change tested? Docs have been tested locally with `make run` in `/docs`.
Issue
There is no documentation about how to perform minor upgrades and rollbacks on PostgreSQL 16.
Solution
I tried to make it easy to skim through, but still emphasizing critical points. I avoided large blocks of text in favor of referencing other documentation and resources.
It does not cover every single nuance, but my intention is to merge a first iteration of refresh documentation as soon as possible so that users have something to part from. I expect it to grow and evolve in future PRs.
Checklist
DPE-7949