-
Notifications
You must be signed in to change notification settings - Fork 98
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
Correct external DB instructions for foreman-{deb,el} #2396
base: master
Are you sure you want to change the base?
Conversation
101077d
to
335efd3
Compare
I noticed that the PostgreSQL path we use is for Debian 11, which uses PostgreSQL 13. Ubuntu 20.04 uses PostgreSQL 12, so the path is different. This may need a bit more work. |
acca56d
to
04b49bf
Compare
. xref:preparing-a-host-for-external-databases_{context}[]. | ||
// TODO: a {RHEL} vs an {EL} | ||
Prepare a {RHEL} 8 server to host the external databases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prepare a {RHEL} 8 server to host the external databases. | |
Prepare a {EL} 8 server to host the external databases. |
:EL: {RHEL} |
:EL: Enterprise Linux |
Will this help for your TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's exactly why there is a TODO. It would be an Enterprise Linux
(correct) and an RHEL
(incorrect).
One solution would be to create a new macro {an-EL}
or similar that does the right thing, but I'm not sure what's best.
@maximiliankolb any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, the {EL}
attribute satisfies both upstream and downstream, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: "Prepare your {EL} host ..."? I know this is a really ugly issue; it also happens with "a orcharhino Proxy"; sometimes even as "{SmartProxy}s -> orcharhino Proxys" instead of "Proxies" (we have an attribute for the latter by now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on side stepping it and saying a server
instead? Then leave it to the individual section to prepare the server? Looking ahead when we support both EL 8 & 9 at the same time that may (or may not) be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"server" is also OK for me. Any thoughts @asteflova ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with "server".
(I would actually prefer "server" regardless of the an/a RHEL/EL issue: No need to specify particular details here because those details are included in the procedure you link to.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO Server will be more generic. Decrease the readability score. We should play with attributes even if it applies to all builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with Aneta on reducing the details in the sentence and saying just "Prepare a server...". Another option would be "Prepare a server on {EL} 8 to host...".
guides/common/modules/con_postgresql-as-an-external-database-considerations.adoc
Outdated
Show resolved
Hide resolved
|
||
.Procedure | ||
|
||
. On {ProjectServer}, stop {Project} services: | ||
. On {ProjectServer}, stop services: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be specific about the type of services we are stopping? As there can be many other active services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. I wasn't sure. To me it felt odd because PostgreSQL is considered a project service, but we exclude it. And you could write out stop {Project} services except PostgreSQL
but I like short instructions when the actual command is self explanatory. However, I realize I may not be representative for the target audience so I'm fine with whatever the writers suggest is best here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend the original one. What's your take @maximiliankolb ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. On {ProjectServer}, stop services: | |
. On {ProjectServer}, stop all {Project} services except for PostgreSQL: |
Unsure about "except for"; maybe just "except"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except for PostgreSQL
guides/common/modules/proc_migrating-to-external-databases.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_migrating-to-external-databases.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_configuring-satellite-to-use-external-databases.adoc
Outdated
Show resolved
Hide resolved
// TODO: a {RHEL} vs an {EL} | ||
Prepare a {RHEL} 8 server to host the external databases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: a {RHEL} vs an {EL} | |
Prepare a {RHEL} 8 server to host the external databases. | |
Prepare your {RHEL} 8 server to host the external databases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a big overlap between this and the assembly. Would a snippet be a good idea?
|
||
.Procedure | ||
|
||
. On {ProjectServer}, stop {Project} services: | ||
. On {ProjectServer}, stop services: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. On {ProjectServer}, stop services: | |
. On {ProjectServer}, stop all {Project} services except for PostgreSQL: |
Unsure about "except for"; maybe just "except"?
---- | ||
# {package-remove} {postgresql-server-package} | ||
---- | ||
+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ |
@@ -14,8 +14,11 @@ endif::[] | |||
|
|||
To create and use external databases for {Project}, you must complete the following procedures: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To create and use external databases for {Project}, you must complete the following procedures: | |
To create and use external databases for {Project}, complete the following procedures: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a very common pattern:
$ rg 'you must' | wc -l
188
While I agree with your suggestion, should it be tackled project wide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree with your suggestion, should it be tackled project wide?
That might be risky. Consider cases like "you must not". Removing "you must" in bulk would break sentences like that.
While we could easily exclude individual cases like these ^, there might be others. We would need to come up with an exhaustive list, and I don't know how we would go about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that is just one instance:
$ rg 'you must' | grep -v 'you must not' | wc -l
187
In this case I can take care of it, but I'm cautious about blowing up the scope too much.
e634158
to
b7d16b2
Compare
I've taken a bit of time to split the commits into more logical changes. Each individual commit should be reasonably easy to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, fix the build first. I'd like to take a look at the output too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name this snip_steps-external-db-migration.adoc
instead?
I had some difficulty with the size of this PR and I've opened #2514 with some the things that I are ready to be merged. |
This way they are all consistent with eachother. They follow: * manage * host * name * user * password Where parameters were missing they are added.
Also uses the package-install attribute.
After the DB is unmanaged and no longer needed, it should be removed to free up resources.
Turns out the snippet isn't exactly the same: one has xref, the other doesn't. |
When Katello isn't present, you don't need the Candlepin and Pulp databases. With this change, the scenario becomes correct and can be shown. This also adds steps to remove the existing database on the Foreman server, freeing up resources.
b7d16b2
to
a38cbf8
Compare
Will rebase once #3167 is merged. |
When Katello isn't present, you don't need the Candlepin and Pulp databases. With this change, the scenario becomes correct and can be shown.
This is now a draft since I'm not entirely sure if it's all correct. I'm going to rely on the preview to tell me that.
Please cherry-pick my commits into: