Add new build target for foremanctl #4506
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds new foremanctl-specific build targets for katello, orcharhino, and satellite, wires them into CI, site navigation, and nightly metadata, introduces corresponding attribute/repository snippets, refactors key guides to conditionally render foremanctl vs legacy installer content without tabs, and removes the previously separate foremanctl-specific Installing Server guide in favor of the new build-target approach. Flow diagram for build target selection and foremanctl content renderingflowchart TD
A[Start make html] --> B[Iterate over build targets]
B --> C{BUILD value}
C --> D[foreman-el]
C --> E[foreman-deb]
C --> F[foremanctl-katello]
C --> G[foremanctl-orcharhino]
C --> H[foremanctl-satellite]
C --> I[katello]
C --> J[satellite]
C --> K[orcharhino]
subgraph Guides_build
D --> L[Use common attributes-foreman-el.adoc]
E --> M[Use common attributes-foreman-deb.adoc]
F --> N[Use attributes-foremanctl-katello.adoc]
G --> O[Use attributes-foremanctl-orcharhino.adoc]
H --> P[Use attributes-foremanctl-satellite.adoc]
I --> Q[Use attributes-katello.adoc]
J --> R[Use attributes-satellite.adoc]
K --> S[Use attributes-orcharhino.adoc]
end
subgraph Repository_snippets
N --> N1[Use snip_configuring-repositories-foremanctl-katello.adoc]
O --> O1[Use snip_configuring-repositories-foremanctl-orcharhino.adoc]
H --> P1[Use snip_configuring-repositories-foremanctl-satellite.adoc]
Q --> Q1[Use snip_configuring-repositories-el.adoc]
R --> R1[Use snip_configuring-repositories-el.adoc]
S --> S1[Use snip_configuring-repositories-el.adoc]
end
subgraph Quickstart_and_installing_guides
N1 --> T{Is BUILD foremanctl flavor}
O1 --> T
P1 --> T
L --> T
M --> T
Q1 --> T
R1 --> T
S1 --> T
T --> U[Yes: render foremanctl content only]
T --> V[No: render legacy installer content]
end
U --> W[HTML output with foremanctl guides]
V --> X[HTML output with legacy installer guides]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
Needs tech review
aneta-petrova
removed
Needs tech review
aneta-petrova
force-pushed
the
foremanctl-flavor
branch
from
7a6533b to
78778e9
Compare
|
Hi @ekohl and @jafiala, this is the result of what we discussed earlier today -- a new build target for foremanctl docs:
As @maximiliankolb correctly pointed out in #3975 (comment), any line that's currently included with other conditionals (such as Please let me know what you think. The point of this PR is to investigate whether this approach will scale in the future (compared to the other alternatives described in this PR's description) so feedback is welcome. |
aneta-petrova
force-pushed
the
foremanctl-flavor
branch
from
78778e9 to
6386d76
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
This indeed looks very much like what we discussed and makes it easy for others to see what we're talking about. I'm positively surprised how small the whole diff really is.
aneta-petrova
force-pushed
the
foremanctl-flavor
branch
2 times, most recently
from
a9dc74e to
d7070f0
Compare
|
In #4506 (comment), @ekohl wrote this:
In the latest commit, I added some content to the server installation guide: https://theforeman-foreman-documentation-preview-pr-4506.surge.sh/nightly/Installing_Server/index-foremanctl.html Because there is just one "foremanctl" build target, this would mean that all of our existing build targets (Foreman on EL, Foreman on Deb, Foreman+Katello, downstreams) would share one preview upstream for foremanctl. Per @ekohl's idea, the foremanctl target is where we would be creating foremanctl content (which would be the same for all existing build targets in a containerized world?), and then add the foremanctl attribute to those existing build targets when ready. However, this means that this foremanctl build will not respect conditionals for our existing build targets. In other words, the foremanctl build target will not show content within conditionals such as In the other PR I raised, #4507, I also added the same server installation guide content. Except that in that PR, each existing build target (EL, Deb, Katello, downstreams) get their own foremanctl-friendly build targets and previews:
This means that we can add foremanctl conditionals while still respecting the conditionals for existing build targets (for example, a katello-foremanctl preview will still show all content with Based on my knowledge of the foremanctl-powered future (which, I admit, is undoubtedly incomplete), I believe the second approach fits our needs better because we can add foremanctl bits and pieces to our existing build targets, and always have a reliable preview for each of them. |
aneta-petrova
force-pushed
the
foremanctl-flavor
branch
5 times, most recently
from
e97dedb to
9b9f171
Compare
aneta-petrova
force-pushed
the
foremanctl-flavor
branch
from
1c51d66 to
b8c081f
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
htmltarget in the rootMakefileonly buildsforemanctl-katello, but the README and deploy workflow list all threeforemanctl-*contexts; align these so the documented behavior, CI build, and localmake htmloutput are consistent. - There is a fair amount of repetition between the new
attributes-foremanctl-*andsnip_configuring-repositories-foremanctl-*files and existing variants; consider extracting shared content into common includes to reduce duplication and the risk of future drift between flavors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `html` target in the root `Makefile` only builds `foremanctl-katello`, but the README and deploy workflow list all three `foremanctl-*` contexts; align these so the documented behavior, CI build, and local `make html` output are consistent.
- There is a fair amount of repetition between the new `attributes-foremanctl-*` and `snip_configuring-repositories-foremanctl-*` files and existing variants; consider extracting shared content into common includes to reduce duplication and the risk of future drift between flavors.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I've made some changes to the initial PoC based on conversations with @ekohl and @maximiliankolb. The PR's description has been updated to reflect that and the PR itself is now ready for review. I'm looking forward to more comments, feedback, and thoughts. To summarize: In this PR, we are looking at what the future documentation structure will look like and how we can make sure it efficiently reflects the future state of things in the Foreman+Katello world as described in #4506 (comment). |
|
Yes, I meant #4517. That's what I get for quickly trying to wrap up something before a meeting. |
maximiliankolb
left a comment
maximiliankolb
left a comment
There was a problem hiding this comment.
Thanks Anet, LGTM!
Includes switching Quickstart from tabs to a separate foremanctl guide
It's now obsolete because we're going with a new flavor rather than a new separate guide.
aneta-petrova
force-pushed
the
foremanctl-flavor
branch
from
34d936f to
4414795
Compare
README.md
Outdated
| To build both the static site and the guides for easy local testing, a global `Makefile` is provided in the root directory with the following targets: | ||
|
|
||
| * `html`: builds HTML guides with all contexts (`foreman-el`, `foreman-deb`, `katello`, `satellite`, and `orcharhino`) | ||
| * `html`: builds HTML guides with all contexts (`foreman-el`, `foreman-deb`, `foremanctl-katello`, `foremanctl-orcharhino`, `foremanctl-satellite`, `katello`, `satellite`, and `orcharhino`) |
There was a problem hiding this comment.
The build target only depends on build-foreman-el build-foreman-deb build-foremanctl-katello build-katello -- so no satellite and no orchanirno (neither classic nor foremanctl), which one is correct?
There was a problem hiding this comment.
I went with the definition in the README: builds HTML guides with all contexts so I added the missing items to the makefile.
evgeni
left a comment
evgeni
left a comment
There was a problem hiding this comment.
it'd be cool if readme vs makefile definition would be resolved, but it's not a new issue, so I'm OK with ignoring it
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
One thing I noticed: in the "affected files" comment: we link to removed files too. That was a bit confusing. It's rather rare we remove files so probably not important to address.
I think this is now largely doing what we want and a solid base to build on. Now the the hard part starts: actually filling the content.
Resolved now (by extending the line in the makefile). I'll leave space for further comments and merge tomorrow if there aren't any. Thanks everyone! |
This fixes previews. Fixes: b27cba4 ("Add new build target for foremanctl (theforeman#4506)")
4 participants
What changes are you introducing?
foremanctlflavor for build targetskatello,orcharhino, andsatelliteforemanct-katellobuild target to web navigation. The list of guides on the website now includes Quickstart and skeleton previews of two high-impact guides: server installation and upgrading (both empty, to be filled with foremanctl content later)Why are you introducing these changes? (Explanation, links to references, issues, etc.)
To prepare the underlying documentation structure for developing
foremanctldocumentation in parallel to documentation based on the existing installer.The expected future state when
foremanctlbecomes more mature is described in #4506 (comment) This documentation PR is developed to support that future, while also allowing downstream teams to continuously review the state of their foremanctl-powered documentation sets.Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Up until now, foremanctl documentation has been developed with the expectation that with a future version, Foreman will switch from using the current Puppet-based installer to the new containerized deployment tool named foremanctl. This is why in a previous PR #3975, a new guide directory was created, with the expectation that it would at a certain point replace the existing guide.
Now it's become clear that the switch to foremanctl will be more gradual upstream and for some time, foremanctl will exists alongside non-foremanctl builds. This PR was raised to investigate an alternative approach where a new build target is created instead of a new separate guide.
Alternative approaches that have been considered:
Contributor checklists
Please cherry-pick my commits into:
Summary by Sourcery
Introduce foremanctl-specific build targets and documentation structure alongside existing installer-based guides.
New Features:
Enhancements:
Build:
CI:
Documentation: