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] Minor tidying for ddev pull platform PRs #4441 and #4426 #4469

Merged
merged 6 commits into from Dec 20, 2022

Conversation

mattstein
Copy link
Sponsor Collaborator

@mattstein mattstein commented Dec 19, 2022

The Problem/Issue/Bug:

Documentation guy is too slow right now and trying to catch up.

How this PR Solves The Problem:

This makes some minor edits for consistency and hopefully clearer phrasing about how databases are handled in the Platform.sh integration.

I brought this up to date with the master branch and had to fix a conflict in docs/content/users/providers/platform.md, which I think looks good and doesn’t omit any improvements. But you may want to double-check.

Manual Testing Instructions:

Preview changes locally or inspect the automatic build:

Automated Testing Overview:

n/a

Related Issue Link(s):

Release/Deployment notes:

n/a

- Replace “or” with comma for consistency.
- Capitalize “Bash”.
- Remove extra line break.
# Conflicts:
#	docs/content/users/providers/platform.md
@mattstein mattstein requested a review from rfay December 19, 2022 19:51
Copy link
Member

@rfay rfay 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 the multiple efforts on this. It's hard (and was hard to implement :) ).

But since it only matters to people with more than one db, it probably should be skippable by ordinary folks.

I should have spent more time explaining the 3 cases:

  1. One database upstream: Gets downloaded into 'db', nothing required of the user, they don't have to even think. This is the most common case.
  2. Multiple databases upstream, PLATFORM_PRIMARY_RELATIONSHIP set: PLATFORM_PRIMARY_RELATIONSHIP's db goes into 'db'. All other databases are pulled and they are named as they were upstream.
  3. Multiple databases upstream, PLATFORM_PRIMARY_RELATIONSHIP not set: All databases are downloaded and loaded into databases with the same names they have upstream.

@@ -550,7 +550,7 @@ Flags:

* `--all`: List unofficial *and* official add-ons. (default `true`)
* `--list`: List official add-ons. (default `true`)
* `--verbose` or `-v`: Output verbose error information with bash `set -x` (default `false`)
* `--verbose`, `-v`: Output verbose error information with Bash `set -x` (default `false`)
Copy link
Member

Choose a reason for hiding this comment

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

I have a hard time capitalizing 'Bash' but can live with it.

The reality is I have a hard time with 'DDEV' :)

Do this by setting PLATFORM_PRIMARY_RELATIONSHIP, for example,
### Designating a Primary Database

All databases for your Platform.sh project will be pulled locally, with each name matching its remote. When you have more than one, you can optionally tell DDEV which to use as the primary and load into the default `'db'` database.
Copy link
Member

Choose a reason for hiding this comment

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

Since this won't matter to the vast majority of people, we may want to calm it instead of making it a major header. That's why it was calmed into bullets before; it doesn't matter most of the time, most people have one database.

Suggested change
All databases for your Platform.sh project will be pulled locally, with each name matching its remote. When you have more than one, you can optionally tell DDEV which to use as the primary and load into the default `'db'` database.
If you have just one database on Platform.sh, it will be pulled and loaded into the local default database named 'db'.
However, if you have more than one database on platform.sh,
all databases will be pulled locally, with each name matching its remote. If you have more than one, you can optionally tell DDEV which to use as the primary one and load into the default `'db'` database.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

I was attempting to serve the same goal with my edit, using a subheading that would immediately signal to someone they could skip the section, and with the first sentence again reiterating that it’s a situation for multiple project databases only.

While this visually softens it, I’d argue that it assumes someone will read everything here and makes it harder to skip.

In the interest of clarity and completeness, though, I like your list above and would rather rewrite this into a “Managing Multiple Databases” section that blends your three-point list above with the example here designating a primary.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with however you want to do it, knowing what you know. Thanks! Sorry this one is so painful.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

Not at all, it’s an interesting feature to describe simply! I’ll counter-suggest something again shortly.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

Alright, another stab: 1c0d63e

I used my theoretical understanding to try and clarify why you may want to designate a primary database, since it’s optional.

@rfay rfay changed the title [docs] Minor tidying for #4441 and #4426 [docs] Minor tidying for ddev pull platform PRs #4441 and #4426 Dec 20, 2022
Clarify:

- Default, single-database behavior.
- Multiple-database behavior.
- Optional primary designation, with an example of when it could be useful.
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks!

@rfay rfay merged commit 68309ef into ddev:master Dec 20, 2022
@mattstein mattstein deleted the docs/4441-4426-tidying branch December 20, 2022 21:22
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.

None yet

3 participants