Skip to content

Fixes #38785 - Align docs links with core's documentation versions#1008

Merged
adamruzicka merged 1 commit intotheforeman:masterfrom
kfamilonidis:docs-link-url
Oct 20, 2025
Merged

Fixes #38785 - Align docs links with core's documentation versions#1008
adamruzicka merged 1 commit intotheforeman:masterfrom
kfamilonidis:docs-link-url

Conversation

@kfamilonidis
Copy link
Copy Markdown
Contributor

@kfamilonidis kfamilonidis commented Sep 29, 2025

Acceptance Criteria

Links are adjusted to core docs.

  • Monitor -> Jobs -> link (path: /job_invocations)
    • empty list: Learn more about this in the documentation link
    • non-empty list: Documentation button
  • Hosts -> Templates -> Job Templates -> link (path: /job_templates)
    • non-empty list: Documentation button

Removes use of :root_url in favor of foreman's core docs params.

Fixes df6ffa2 (Fixes #14560 - Reuse foreman's core documentation helper. (#171))

@MariaAga
Copy link
Copy Markdown
Member

I think what broke it was that we moved pages around in the docs and not #171, as this was only broken for the last 1/2 versions

@kfamilonidis
Copy link
Copy Markdown
Contributor Author

it seems that linking of the documentation pages is in a middle of a transition phase, and in this specific case there was a completely unaligned link with the current documentation version, In the API itself, there is obvious the use of :root_url that feels that is not the most up-to-date method of referencing the documentations' pages.

@MariaAga
Copy link
Copy Markdown
Member

I asked Ewoud in the Jira ticket, but copying it here for visibility:
In https://github.com/theforeman/foreman_remote_execution/pull/818/files

The welcome page/empty page for job invocations was changed to https://docs.theforeman.org/nightly/Managing_Hosts/index-foreman-el.html#Configuring_and_Setting_Up_Remote_Jobs_managing-hosts

Should it be consistent or should the index page point somewhere else? (#executing-a-remote-job_managing-hosts as you suggested)

@kfamilonidis kfamilonidis force-pushed the docs-link-url branch 3 times, most recently from 2ebc6dc to 24bdc4d Compare October 7, 2025 10:57
@kfamilonidis
Copy link
Copy Markdown
Contributor Author

this is now waiting for approval PR

Copy link
Copy Markdown
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.

Similar to the other PR, please describe what the change does, not what it solves.

url = 'http://theforeman.org/plugins/foreman_remote_execution/' +
"#{ForemanRemoteExecution::VERSION.split('.').take(2).join('.')}/index.html#"
documentation_button section, :root_url => url
def documentation_button_rex(chapter = '')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is also used in app/views/job_templates/index.html.erb and that needs to be modified too.

Now looking at the whole repo, is there any real point in keeping this helper? It's used twice and can we really assume that all the documentation will be in the Managing_Hosts guide? I'd suggest dropping it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks for the suggestion. I also was between keeping this helper and wasn't sure if this helper pattern was used into other repo. It now make more sense since the core :params are stable.

Also, at the job_templates/index.erb, the new link is included, but it needs a different :key in order to be activated here. Thanks for this also!

@MariaAga
Copy link
Copy Markdown
Member

MariaAga commented Oct 7, 2025

I asked Ewoud in the Jira ticket, but copying it here for visibility:
In https://github.com/theforeman/foreman_remote_execution/pull/818/files
The welcome page/empty page for job invocations was changed to https://docs.theforeman.org/nightly/Managing_Hosts/index-foreman-el.html#Configuring_and_Setting_Up_Remote_Jobs_managing-hosts
Should it be consistent or should the index page point somewhere else? (#executing-a-remote-job_managing-hosts as you suggested)

Should https://github.com/theforeman/foreman_remote_execution/blob/master/app/views/job_invocations/welcome.html.erb
reuse the documentation_button_rex?

@kfamilonidis kfamilonidis changed the title Fixes #38785 - documentation link on the Jobs page links into a 404 Fixes #38785 - Align docs links with core's documentation versions Oct 8, 2025
@kfamilonidis
Copy link
Copy Markdown
Contributor Author

Should https://github.com/theforeman/foreman_remote_execution/blob/master/app/views/job_invocations/welcome.html.erb
reuse the documentation_button_rex?

I removed documentation_button_rex altogether. empty-state link is a <anchor> tag, non-empty state links are <button> tags

@kfamilonidis kfamilonidis requested a review from ekohl October 8, 2025 09:00
Copy link
Copy Markdown
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.

I'll leave this for others for a final review.


<% title _("Job Templates") %>
<% title_actions(documentation_button_rex('3.1JobTemplates'),
<% title_actions(documentation_button('Managing_Hosts', type: 'docs', chapter: 'creating-a-job-template_managing-hosts'),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@aneta-petrova This may not be actionable in the moment, but in the old documentation we had a sort of "landing page" for job templates (https://theforeman.org/plugins/foreman_remote_execution/1.7/index.html#3.1JobTemplates). Now we link to https://docs.theforeman.org/3.16/Managing_Hosts/index-katello.html#creating-a-job-template_managing-hosts even though the page also allows importing etc.

Perhaps this is a nice hook into the REX content journey you had in mind.

Remove use of :root_url in favor of Foreman's core docs params.

This fixes df6ffa2 (Fix #14560 - Reuse Foreman's core documentation helper (theforeman#171))
Copy link
Copy Markdown
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

LGTM


<% title _("Job Templates") %>
<% title_actions(documentation_button_rex('3.1JobTemplates'),
<% title_actions(documentation_button('Managing_Hosts', type: 'docs', chapter: 'creating-a-job-template_managing-hosts'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is slightly unfortunate that including the type keyword argument leads to warnings like these in the logs, but that probably isn't something we could fix here

2025-10-14T10:45:28 [I|app|35819b1a] Started GET "/links/docs/Managing_Hosts?chapter=creating-a-job-template_managing-hosts" for 10.44.34.27 at 2025-10-14 10:45:28 -0400
2025-10-14T10:45:28 [I|app|35819b1a] Processing by LinksController#show as HTML
2025-10-14T10:45:28 [I|app|35819b1a]   Parameters: {"chapter"=>"creating-a-job-template_managing-hosts", "type"=>"docs", "section"=>"Managing_Hosts"}
2025-10-14T10:45:28 [D|app|35819b1a] Unpermitted parameter: :type. Context: { controller: LinksController, action: show, request: #<ActionDispatch::Request:0x00007efc9a082620>, params: {"chapter"=>"creating-a-job-template_managing-hosts", "controller"=>"links", "action"=>"show", "type"=>"docs", "section"=>"Managing_Hosts"} }
2025-10-14T10:45:28 [I|app|35819b1a] Redirected to https://docs.theforeman.org/nightly/Managing_Hosts/index-katello.html#creating-a-job-template_managing-hosts
2025-10-14T10:45:28 [I|app|35819b1a] Completed 302 Found in 2ms (ActiveRecord: 0.3ms | Allocations: 974)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct. I had noticed this and made this change here. It fixes the warning.

@adamruzicka adamruzicka merged commit a6997a8 into theforeman:master Oct 20, 2025
25 checks passed
@adamruzicka
Copy link
Copy Markdown
Contributor

Thank you @kfamilonidis !

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.

4 participants