Skip to content

Updating find_unused_modules script #3760

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

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

apinnick
Copy link
Contributor

@apinnick apinnick commented Apr 1, 2025

What changes are you introducing?

Added a line to detect snippets included in modules.

Added a comment that script does not work on file includes that contain attributes. User will have to ignore those files in the output.

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

Snippets included in modules were reported as false positives.

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

I tested this with a fake snippet. It was detected.

No style review required.

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.14/Katello 4.16
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16)
  • 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)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • We do not accept PRs for Foreman older than 3.7.

@apinnick apinnick requested a review from maximiliankolb April 1, 2025 10:30
@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 Apr 1, 2025
Copy link

github-actions bot commented Apr 1, 2025

The PR preview for c4b9779 is available at theforeman-foreman-documentation-preview-pr-3760.surge.sh

No diff compared to the current base

show diff

@apinnick apinnick removed the Needs style review Requires a review from docs style/grammar perspective label Apr 1, 2025
@apinnick apinnick marked this pull request as ready for review April 1, 2025 10:45
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.

Tested locally; it works as expected:

$ ./guides/scripts/find_unused_modules.py
proc_configuring-repositories-katello.adoc
proc_configuring-repositories-foreman-el.adoc
proc_configuring-repositories-satellite.adoc
proc_configuring-repositories-orcharhino.adoc
$ echo $?
0

There are still false positives but this is much much better than before. Thanks Avital.

@maximiliankolb maximiliankolb added tech review done No issues from the technical perspective testing done No issues from the functional perspective and removed Needs tech review Requires a review from the technical perspective Needs testing Requires functional testing labels Apr 1, 2025
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Something that I came across when playing with #3741 was the :sourcemap option in https://docs.asciidoctor.org/asciidoctor/latest/api/options/

Tracks the file and line number for each parsed block. Useful for tooling applications where the association between the converted output and the source file is important.

https://docs.asciidoctor.org/asciidoctor/latest/api/sourcemap/ is a longer document that shows asciidoctor internally keeps track of all the files it uses. It actually made me think about creating debug outputs where we display within the rendered guide the actual source location for each paragraph.

Taking it back to this script: would it make sense to adopt a strategy of actually rendering all guides in all flavors and then look at which files it actually used? That way we're not guessing. It will also catch edge cases where we use variables in filenames.

@apinnick
Copy link
Contributor Author

apinnick commented Apr 1, 2025

Something that I came across when playing with #3741 was the :sourcemap option in https://docs.asciidoctor.org/asciidoctor/latest/api/options/

Tracks the file and line number for each parsed block. Useful for tooling applications where the association between the converted output and the source file is important.

https://docs.asciidoctor.org/asciidoctor/latest/api/sourcemap/ is a longer document that shows asciidoctor internally keeps track of all the files it uses. It actually made me think about creating debug outputs where we display within the rendered guide the actual source location for each paragraph.

Taking it back to this script: would it make sense to adopt a strategy of actually rendering all guides in all flavors and then look at which files it actually used? That way we're not guessing. It will also catch edge cases where we use variables in filenames.

@ekohl Pity we didn't know about this while we were still committed to Asciidoc! 😄

At the moment, it's probably best to keep our tools as generic as possible. The effects of the d/s tooling changes are still a mystery. When we know more, we can think about updating our tools.

@ekohl
Copy link
Member

ekohl commented Apr 1, 2025

I couldn't help myself:

#!/usr/bin/env ruby

# frozen_string_literal: true

require 'asciidoctor'

BUILDS = %w[
  foreman-el
  foreman-deb
  katello
  satellite
  orcharhino
].freeze

BASE_DIR = File.dirname(__dir__)

included = Set.new

Dir.glob(File.join(BASE_DIR, '*', 'master.adoc')).each do |path|
  BUILDS.each do |build|
    doc = Asciidoctor.load_file(
      path,
      doctype: :book,
      safe: :safe,
      base_dir: BASE_DIR,
      attributes: { 'build' => build }
    )

    included += doc.catalog[:includes].keys
  end
end

all_modules = Dir.glob(File.join(BASE_DIR, 'common', 'modules', '**', '*.adoc'))
absolute_included = included.map { |path| File.join(BASE_DIR, "#{path}.adoc") }

puts all_modules - absolute_included
$ guides/scripts/find_modules
asciidoctor: ERROR: master.adoc: line 5: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman.adoc
asciidoctor: ERROR: master.adoc: line 21: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman-contributors.adoc
asciidoctor: ERROR: master.adoc: line 5: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman.adoc
asciidoctor: ERROR: master.adoc: line 21: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman-contributors.adoc
asciidoctor: ERROR: master.adoc: line 11: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman.adoc
asciidoctor: ERROR: master.adoc: line 12: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/katello.adoc
asciidoctor: ERROR: master.adoc: line 21: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman-contributors.adoc
asciidoctor: ERROR: master.adoc: line 28: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/katello-contributors.adoc
asciidoctor: ERROR: master.adoc: line 5: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman.adoc
asciidoctor: ERROR: master.adoc: line 21: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman-contributors.adoc
asciidoctor: ERROR: master.adoc: line 5: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman.adoc
asciidoctor: ERROR: master.adoc: line 21: include file not found: /home/ekohl/dev/foreman-documentation/guides/topics/foreman-contributors.adoc
/home/ekohl/dev/foreman-documentation/guides/common/modules/con_configuring-keycloak-wildfly-authentication-with-piv-cards-for-project.adoc
/home/ekohl/dev/foreman-documentation/guides/common/modules/snip_prerequisites-content-for-sles.adoc

This is a different approach. I'm ignoring the errors from the release notes for now.

It shows a false positive for snip_prerequisites-content-for-sles.adoc, probably because the way we render orcharhino isn't quite correct.

Then it finds con_configuring-keycloak-wildfly-authentication-with-piv-cards-for-project.adoc which is unused. It's included in assembly_configuring-keycloak-wildfly-authentication-with-piv-cards-for-project.adoc but that whole assembly itself appears to be unused.

@apinnick apinnick merged commit da2113a into theforeman:master Apr 2, 2025
7 of 8 checks passed
@apinnick apinnick deleted the modules-script branch April 2, 2025 06:56
@ekohl
Copy link
Member

ekohl commented Apr 16, 2025

This is a different approach. I'm ignoring the errors from the release notes for now.

That's because I'm setting the base_dir incorrectly:

      base_dir: BASE_DIR,

This should be base_dir: File.dirname(path).

The whole file:

#!/usr/bin/env ruby

# frozen_string_literal: true

require 'asciidoctor'

BUILDS = %w[
  foreman-el
  foreman-deb
  katello
  satellite
  orcharhino
].freeze

BASE_DIR = File.dirname(__dir__)

included = Set.new

Dir.glob(File.join(BASE_DIR, '*', 'master.adoc')).each do |path|
  BUILDS.each do |build|
    doc = Asciidoctor.load_file(
      path,
      doctype: :book,
      safe: :safe,
      base_dir: File.dirname(path),
      attributes: { 'build' => build }
    )

    included += doc.catalog[:includes].keys
  end
end

all_modules = Dir.glob(File.join(BASE_DIR, 'common', 'modules', '**', '*.adoc'))
absolute_included = included.map { |path| File.join(BASE_DIR, "#{path}.adoc") }

puts all_modules - absolute_included

Now the output is:

$ guides/scripts/find_modules 
/home/ekohl/dev/foreman-documentation/guides/common/modules/con_configuring-keycloak-wildfly-authentication-with-piv-cards-for-project.adoc
/home/ekohl/dev/foreman-documentation/guides/common/modules/snip_prerequisites-content-for-sles.adoc

@ekohl ekohl mentioned this pull request Apr 16, 2025
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech review done No issues from the technical perspective testing done No issues from the functional perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants