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

Split proc_configuring-repositories.adoc into several files #3123

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jul 11, 2024

The procedure to enable repositories is very specific to every build flavor. Trying to squeeze this into a single file makes it very complicated. This makes maintenance easier and doesn't really duplicate that much.

This PR is split off from #3104 to verify it's just an internal refactor and doesn't have any impact on the resulting guides. It may also be easier to review this standalone, but I'll leave that up to others.

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.11/Katello 4.13
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

Copy link

github-actions bot commented Jul 11, 2024

The PR preview for a66f856 is available at theforeman-foreman-documentation-preview-pr-3123.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@ekohl ekohl force-pushed the split-configuring-repositories branch from 87e8a0d to cedf82d Compare July 11, 2024 14:21
@ekohl ekohl marked this pull request as ready for review July 11, 2024 14:25
@ekohl
Copy link
Member Author

ekohl commented Jul 11, 2024

This PR is split off from #3104 to verify it's just an internal refactor and doesn't have any impact on the resulting guides. It may also be easier to review this standalone, but I'll leave that up to others.

After fixing one mistake, you can now see that the diff is empty. Just as intended.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Please rename the file to a) use "snip_" as prefix because the files don't contain any heading; and b) maybe use the build target in the file name: example: snip_configuring-repositories-katello.adoc. Then, you can always, without any ifdef macro, include snip_configuring-repositories-{build}.adoc similar to https://github.com/theforeman/foreman-documentation/blob/master/guides/common/attributes.adoc?plain=1#L51

guides/common/modules/proc_configuring-repositories.adoc Outdated Show resolved Hide resolved
@ekohl ekohl requested a review from asteflova July 12, 2024 08:49
@asteflova
Copy link
Contributor

I'm not a huge fan of the idea to split off intros from procedure modules. As far as contributor experience goes, that's a change for the worse. Then again, I'm not a fan of splitting off assembly introductions into separate files either, and I still found a way to live with that, so... :)

The idea behind modularity is to enable reuse -- so you're supposed to split off "reusable" pieces of content, pieces that can stand on their own and that make sense on their own[1]. Because you can't reuse .Procedure without its intro and vice versa, then as far as the modular documentation framework goes, this would mean bending the rules quite a bit. Whether that's a problem or not depends on the value that it would bring to implementing tabs.

[1] "A module must make sense and provide value on its own, even when read separately from the other modules. The templates included in this manual help ensure this."
https://redhat-documentation.github.io/modular-docs/#what-modular-documentation-is-not

@ekohl
Copy link
Member Author

ekohl commented Jul 12, 2024

I agree guides/common/modules/proc_configuring-repositories.adoc has become more of an assembly. Should we convert it to an assembly and then include that assembly in multiple places?

The idea behind modularity is to enable reuse -- so you're supposed to split off "reusable" pieces of content, pieces that can stand on their own and that make sense on their own[1]. Because you can't reuse .Procedure without its intro and vice versa, then as far as the modular documentation framework goes, this would mean bending the rules quite a bit. Whether that's a problem or not depends on the value that it would bring to implementing tabs.

While it's true, there is also a point of maintainability. In effect the modules really aren't reusable between build flavors (except foreman-el and katello).

Another way we could structure it is to create modules/proc_configuring-repositories-{{ build }}.adoc and then symlink modules/proc_configuring-repositories-katello.adoc to modules/proc_configuring-repositories-foreman-el.adoc.

Then guides/common/assembly_configuring-repositories.adoc becomes:

[id="configuring-repositories_{context}"]
= Configuring repositories

include::modules/proc_configuring-repositories-{build}.adoc[]

@ekohl ekohl force-pushed the split-configuring-repositories branch 2 times, most recently from 3e87c57 to 0927f17 Compare July 25, 2024 13:39
@ekohl
Copy link
Member Author

ekohl commented Jul 25, 2024

Updated as discussed.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

I would have kept the procedure and used a snippet snip_configuring-repositories-{build}.adoc instead. For now, we don't nest assemblies AFAIK and I'd like to keep it like this. However, I don't want to block this PR either.

Also unrelated: I don't like the current way of using level 2 headings ... Again, not blocking your PR.

What do you think Anet and Avital?

@ekohl
Copy link
Member Author

ekohl commented Jul 26, 2024

For now, we don't nest assemblies AFAIK and I'd like to keep it like this

Yesterday during the meeting it was mentioned that was can be allowed. I interpreted it as something we can do. But I'm fine with refactoring.

@asteflova
Copy link
Contributor

I might not know what others mean by nested assemblies, but I can see plenty of them. One example: 13.3. Using repository sources. So basically anything like ASSEMBLY 3.

I'm not a fan, but I haven't been able to provide reasons for avoiding them that people who are fans would consider valid. (Yet. I'm not giving up.)

@maximiliankolb
Copy link
Contributor

I might not know what others mean by nested assemblies, but I can see plenty of them. One example: 13.3. Using repository sources. So basically anything like ASSEMBLY 3.

I'm not a fan, but I haven't been able to provide reasons for avoiding them that people who are fans would consider valid. (Yet. I'm not giving up.)

I was wrong: cat guides/common/assembly_* | grep assembly returns plenty of results; I thought there would be none. Therefore, this approach has been used before and we should allow PRs that use it. level offset is something different than nesting to me and independent, isn't it?

@maximiliankolb
Copy link
Contributor

GHA fail: asciidoctor: ERROR: common/assembly_configuring-repositories.adoc: line 4: include file not found: /home/runner/work/foreman-documentation/foreman-documentation/guides/doc-Configuring_Load_Balancer/common/modules/proc_configuring-repositories-katello.adoc

@maximiliankolb maximiliankolb added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels Aug 6, 2024
@ekohl ekohl force-pushed the split-configuring-repositories branch from 0927f17 to 25dd59e Compare August 6, 2024 16:37
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author Needs re-review labels Aug 6, 2024
@ekohl
Copy link
Member Author

ekohl commented Aug 6, 2024

GHA fail: asciidoctor: ERROR: common/assembly_configuring-repositories.adoc: line 4: include file not found: /home/runner/work/foreman-documentation/foreman-documentation/guides/doc-Configuring_Load_Balancer/common/modules/proc_configuring-repositories-katello.adoc

Solved with a symlink

@ekohl ekohl requested a review from asteflova August 8, 2024 11:09
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

one tiny suggestion. No diff in rendered HTML is exactly what we expect. So LGTM 👍

The procedure to enable repositories is very specific to every build
flavor. Trying to squeeze this into a single file makes it very
complicated. This makes maintenance easier and doesn't really duplicate
that much.

It effectively transforms the repository setup to an assembly that can
be included.
@ekohl ekohl force-pushed the split-configuring-repositories branch from 25dd59e to a66f856 Compare August 9, 2024 18:55
@ekohl ekohl merged commit 4998b35 into theforeman:master Aug 17, 2024
8 of 9 checks passed
@ekohl ekohl deleted the split-configuring-repositories branch August 17, 2024 10:25
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