-
Notifications
You must be signed in to change notification settings - Fork 95
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
Turn 'deployment considerations' into 'deployment path' #3456
Turn 'deployment considerations' into 'deployment path' #3456
Conversation
Reviewers, please ignore anything under The list of affected guides is long, but only because module reuse is involved. All changes are limited to the Planning guide, chapter named Deployment path for {ProjectName}. Reviewing this chapter should be enough. |
Hi @evgeni, I was told that you might be the right person to give this PR a high-level review. Could you please take a look at My plan is to start asking people for more detailed feedback on the contents of the individual modules later, after this high-level review, but if you want to do that now too (especially for the modules that describe Platform's components), feel free to do so. |
I'm missing two things: compute resources and configuration management tools. While the set of "what is there by default" varies across the different deployment types we offer, there is always the chance to add an "integration with compute provider X" or "integration with configuration management tool Y", and while users will (probably) know that we support a lot of these, calling out "you can add those later" seems like a good thing? |
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.
Thanks Anet, I've reviewed everything but topics/*
. Overall, LGTM.
endif::[] | ||
ifeval::["{context}" == "planning"] | ||
= Configuring external authentication in {Project} | ||
endif::[] |
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.
Not a fan of this construct. I hightly believe that this would break this for me in downstream docs. not a blocker as I can always create a follow-up PR to change this further.
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.
It shouldn't break if you define the context. Can you test it first, please?
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.
Is it the ifeval itself that's an issue, or just using it for a heading right after defining the anchor? We are using ifevals elsewhere already, for example
ifeval::["{context}" == "planning"] |
To be honest, I wasn't totally sure about using ifevals to differentiate headings like this. Won't it be confusing to users if they come across a module that's reused but with a different heading? I think I'll just try to find a heading that will work in both contexts.
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.
@maximiliankolb and I agreed that we will keep this as is for now and submit a follow-up PR if it breaks anything
guides/common/modules/ref_predefined-roles-available-in-project.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/ref_smart-proxy-server-scalability-considerations.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/ref_smart-proxy-server-scalability-considerations.adoc
Show resolved
Hide resolved
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.
Overally looking good. Just a few nitpicks
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
endif::[] | ||
ifeval::["{context}" == "planning"] | ||
= Configuring external authentication in {Project} | ||
endif::[] |
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.
It shouldn't break if you define the context. Can you test it first, please?
03c52fc
to
c501cec
Compare
511fc26
to
69cc838
Compare
Hi @adamruzicka @ianballou @stejskalleos can you please review this PR? The diff looks huge but you don't need to review it all; below, I'm listing which sections are relevant. The goal of this PR is to provide a high-level overview of the steps that users should take during deployment. I want to help users make a few decisions, but link to other parts of the docs set for detailed instructions. This is the new chapter: 7. Deployment path for Foreman / 7. Deployment path for Satellite. Specifically, I'm asking you to review these (mostly very short!) sections that are included in the chapter:
Feel free to look at the others sections too, of course! I'm just trying to make this easier for you so I listed the sections than I think fall into your respective areas of expertise. Please let me know if there is anything unclear. |
Hi @evgeni, you already commented on this PR but I asked you for a high-level review of the whole chapter back then. Do you have any further feedback for sections Installing a Foreman server ( |
For completeness: I'm not asking for a review of section Planning for disaster recovery because right now, that's basically just a list of links. I will be looking at disaster recovery docs in general later and I plan to revisit this section then. |
a4d0371
to
a11a395
Compare
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.
It reads well, but I wasn't sure where it stands with relation to katello. Does it assume pure foreman? If so, should it mention katello somewhere in the additional considerations?
guides/common/modules/con_planning-organization-and-location-context.adoc
Show resolved
Hide resolved
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
The whole guide is currently labeled as Katello-specific. If the guide was intended to cover pure Foreman, you make a good point about the need to mention Katello. I don't think it's something we need to worry about now, though, considering that the guide as a whole doesn't seem to either. Thanks for the feedback, I really appreciate it! |
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.
Thanks for the feedback, Ian!
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
Tech review feedback from all SMEs has been implemented. If anyone has more, please keep it coming, but right now, I think I can go ahead with |
Co-authored-by: Maximilian Kolb <mail@maximilian-kolb.de>
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.
19/29 files.
No blockers (apart from one hardcoded downstream term 🙃 ), mostly suggestions and notes to myself ...
guides/common/modules/con_adding-a-red-hat-subscription-to-your-project-server.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_adding-a-red-hat-subscription-to-your-project-server.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_adding-a-red-hat-subscription-to-your-project-server.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Show resolved
Hide resolved
guides/common/modules/con_defining-content-access-strategies-for-hosts.adoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Maximilian Kolb <mail@maximilian-kolb.de>
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.
27/29. I did not look at the wild table in guides/common/modules/ref_overview-of-authentication-methods-in-project.adoc
and the file under topics/
.
Made some suggestions and notes. Overall, LGTM
guides/common/modules/con_planning-organization-and-location-context.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_planning-organization-and-location-context.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_planning-organization-and-location-context.adoc
Show resolved
Hide resolved
Co-authored-by: Maximilian Kolb <mail@maximilian-kolb.de>
I discussed this PR with @maximiliankolb privately. We agreed that it's good enough to be merged as is. Some comment threads are left open as trackers for him to open subsequent PRs. Therefore, I'll set the Generally speaking, this PR doesn't add new content, it only rearranges, de-duplicates, and shortens existing content. There is an agreement that this PR is an improvement over the current state of things. And we can always continue with further improvements. |
What changes are you introducing?
I'm proposing to get rid of the Deployment Considerations chapter and replace it with a more focused assembly that I named Deployment Path. The new assembly describes actions a user should take to deploy Foreman (installation + typical first configuration steps). Each step in the deployment journey is described only very briefly but it links to other guides for details.
I'm not adding any new information. All that you see in the PR was already documented; mostly in the old Deployment Considerations chapter (which was in some places out-of-date), some pieces I found scattered elsewhere in the docs set. In case of the pieces that were already documented elsewhere, I either copy-pasted them (when it was just one or two sentences) or reused (when it was a whole module, in which case I often had to resort to using ifevals).
Some parts of Deployment Considerations did not fit into the new Deployment Path assembly. I kept them under topics/ and will deal with them later, when modularizing the rest of the guide.
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
All content in Deployment Considerations was stored in topics/, not modules/. The initial goal was to modularize it.
However, if I just split the original large chapter into modules without making any modifications, I would introduce outdated modules into the main file set. So I knew I needed to do some rewriting. From there, it was only one more step towards making the content more action-based.
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
I will need to work with multiple tech reviewers to make sure the content is up-to-date.
Checklists
Please cherry-pick my commits into: