Skip to content

Conversation

@apinnick
Copy link
Contributor

What changes are you introducing?

Removing snip_make-firewall-settings-persistent.adoc and snip_verify-firewall-settings.adoc

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

Snippets that contain a single step are problematic because they introduce potential problems with step numbering. They are also unlikely to be re-used unless a writer is aware of their existence.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

Tech review and testing not required

Contributor checklists

  • 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.16/Katello 4.18 (Satellite 6.18)
  • Foreman 3.15/Katello 4.17
  • Foreman 3.14/Katello 4.16 (Satellite 6.17; orcharhino 7.4)
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16; orcharhino 7.2 on EL9 only; orcharhino 7.3)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9; orcharhino 7.1 with Leapp)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • We do not accept PRs for Foreman older than 3.9.

Review checklists

Tech review (performed by an Engineer who did not author the PR; can be skipped if tech review is unnecessary):

  • The PR documents a recommended, user-friendly path.
  • The PR removes steps that have been made unnecessary or obsolete.
  • Any steps introduced or updated in the PR have been tested to confirm that they lead to the documented end result.

Style review (by a Technical Writer who did not author the PR):

  • The PR conforms with the team's style guidelines.
  • The PR introduces documentation that describes a user story rather than a product feature.

@github-actions github-actions bot added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels Sep 14, 2025
@apinnick apinnick removed Needs tech review Requires a review from the technical perspective Needs testing Requires functional testing labels Sep 14, 2025
@github-actions
Copy link

The PR preview for 0452b61 is available at theforeman-foreman-documentation-preview-pr-4253.surge.sh

No diff compared to the current base

show diff

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.

style-wise LGTM, but as with #4252: I do not fully agree with the notion that the snippets are useless. Is there another reason to drop them? I am OK either way.

@apinnick
Copy link
Contributor Author

apinnick commented Sep 15, 2025

I do not fully agree with the notion that the snippets are useless. Is there another reason to drop them? I am OK either way.

@maximiliankolb

Snippets are not useless! These snippets are. 😄 Very short snippets where the wording does not make much difference to the end user, as long as the content is correct (notes, single procedure steps) require additional maintenance, which is not justified by their limited use.

IMO, snippets are best suited for complex content where the wording matters, like configuration files or legal notices. We use a snippet for a policy.rego file that we have to publish in more than one doc. The complexity of the file justifies the maintenance of a snippet.

@apinnick apinnick merged commit e4692cf into theforeman:master Sep 15, 2025
11 of 12 checks passed
@apinnick apinnick deleted the firewall-persistence-snippet branch September 15, 2025 08:29
@apinnick
Copy link
Contributor Author

Oops, merged that prematurely. Meant to wait to EOD.

@aneta-petrova
Copy link
Member

Oops, merged that prematurely. Meant to wait to EOD.

I suggest to revert this merge because I'm implementing this change as part of a larger review of our procedures to open ports in #4210

@maximiliankolb
Copy link
Contributor

Oops, merged that prematurely. Meant to wait to EOD.

I suggest to revert this merge because I'm implementing this change as part of a larger review of our procedures to open ports in #4210

That's fair. I think it's easiest if you revert this commit on your branch and then keep your existing changes. If you keep the revert directly on top of HEAD of "master", you won't have to resolve any merge conflicts. Does this work for you @aneta-petrova ?

@aneta-petrova
Copy link
Member

@maximiliankolb I'm not sure that would work because my PR needs to be eventually cherry-picked to an earlier version as well (it's part of the installation guide changes) while this PR was merged only to master.

@apinnick Please consider taking one of these two actions: cherry-pick this PR also to 3.16, or revert.

@maximiliankolb
Copy link
Contributor

@maximiliankolb I'm not sure that would work because my PR needs to be eventually cherry-picked to an earlier version as well (it's part of the installation guide changes) while this PR was merged only to master.

After merging two or more commits based on your PR, you could omit cherry-picking the first commit on your branch to 3.16 (and below if necessary). It could work, but I also understand the desire to fix it first/on a different PR.

@aneta-petrova
Copy link
Member

While I could resolve the conflict in my PR, I also think that when this is a result of a recent commit that was merged without adhering to our contribution guidelines, the best path forward is for the person who merged the commit to contribute to a solution. I've hinted at two possible solutions above, but I'm open to any other ideas that @apinnick might have.

apinnick added a commit that referenced this pull request Sep 15, 2025
@apinnick
Copy link
Contributor Author

apinnick commented Sep 15, 2025

I will revert merge. Well, I would revert the merge if the linkchecker did not block me from doing so....

Update: Merged!

apinnick added a commit that referenced this pull request Sep 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs style review Requires a review from docs style/grammar perspective

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants