-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fixes #37041 - Change link to docs.theforeman.org #818
Conversation
@@ -7,7 +7,7 @@ | |||
<%= _("Foreman can run arbitrary commands on remote hosts using different providers, such as SSH or Ansible. Communication goes through the Smart Proxy so Foreman does not have to have direct access to the target hosts and can scale to control many hosts.") %></br> | |||
</p> | |||
<p><%= link_to _('Learn more about this in the documentation.'), | |||
documentation_url('1.ForemanRemoteExecution1.3Manual', :root_url => 'https://www.theforeman.org/plugins/foreman_remote_execution/1.3/index.html#'), :rel => 'external' %></p> | |||
documentation_url('', :root_url => 'https://www.theforeman.org/plugins/foreman_remote_execution/10.0.3/index.html#'), :rel => 'external' %></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.theforeman.org/plugins/foreman_remote_execution/10.0.3/index.html points to nowhere, that page does not exist.
Edit: Latest in there seems to be 1.7. Also why swap the first argument for an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Adam, thanks for taking a look at it. My idea was to not point to any subheading if possible, but instead have a small page referring users to docs.theforeman.org. The URL will be live after the PR in theforeman.org has been merged.
This is IMHO just a temporary workaround until all content from Foreman Manual has been migrated to Foreman Documentation and we have a function to add Documentation buttons in Foreman Web UI and all Foreman plug-ins.
Do you think this is a reasonable approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this helper:
foreman_remote_execution/app/helpers/remote_execution_helper.rb
Lines 205 to 209 in 2b10ad9
def documentation_button_rex(section = '') | |
url = 'http://theforeman.org/plugins/foreman_remote_execution/' + | |
"#{ForemanRemoteExecution::VERSION.split('.').take(2).join('.')}/index.html#" | |
documentation_button section, :root_url => url | |
end |
I think it would be preferable to consistently use the same logic. Perhaps we need a separate helper for root_url
.
It also implies those help buttons are likely to be broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and looking further. A more correct usage of the documentation infrastructure is to use it like foreman_discovery:
https://github.com/theforeman/foreman_discovery/blob/417a7550a1b12da84c4787f8bf9ecf7eebc17ea3/app/views/discovered_hosts/welcome.html.erb#L13
So external_link_path(type: 'plugin_manual', name: 'foreman_discovery', version: discovery_doc_version, section: '#4.3Automaticprovisioning')
(or external_link_url
).
Or in this case:
documentation_url('', :root_url => 'https://www.theforeman.org/plugins/foreman_remote_execution/10.0.3/index.html#'), :rel => 'external' %></p> | |
external_link_path(type: 'plugin_manual', name: 'foreman_remote_execution', version: '1.3', section: '1.ForemanRemoteExecution1.3Manual'), :rel => 'external' %></p> |
And from there update the rest as well.
I've also opened theforeman/foreman#9756 to be able to link to the new docs.
0f26a64
to
3630a6e
Compare
Updated the PR to point to version "10" as applied in theforeman/theforeman.org#2089. I am also OK if we decide to close this PR because we wait until there's a generic way to link to docs.theforeman.org like theforeman/foreman#9756 |
theforeman/foreman#9756 was merged. @maximiliankolb could you please change this PR to use the new things and link to docs.tfm.org? |
4fda4e3
to
4ce793c
Compare
build error from yesterday: cc @adamruzicka My change is ready for review. |
Could you please rebase now that #861 got it? Hopefully it will make packit turn green |
mechanism in Foreman: see PR 9756 in foreman
4ce793c
to
f3b8a95
Compare
Thanks Adam; rebased to master. |
Thank you @maximiliankolb ! |
@adamruzicka will this break branding? |
I must admit I don't know, but I have it written down in my notes to look into it and fix the possible fallout. |
RedHatSatellite/foreman_theme_satellite#32 provides a mechanism to brand these. |
related to theforeman/theforeman.org#2089