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.

Sign up for GitHub

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

Use product settings for docs & links #6718

Merged
merged 1 commit into from
Feb 27, 2020
Merged

Use product settings for docs & links #6718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/presenters/menu/default_menu.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,8 @@ def help_menu_section
:href => '/support/index?support_tab=about'
},
:product => {
:title => I18n.t('product.support_website_text'),
:href => I18n.t("product.support_website").html_safe
:title => ::Settings.docs.product_support_website_text,
:href => ::Settings.docs.product_support_website
},
:about => {
:title => N_('About')
Expand Down
4 changes: 2 additions & 2 deletions app/views/support/show.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
%br
%br
= _("For questions or problem reporting, visit ")
%a{:href => I18n.t("product.support_website").html_safe, :target => "_new"}
= I18n.t("product.support_website_text").html_safe
%a{:href => ::Settings.docs.product_support_website, :target => "_new"}
= ::Settings.docs.product_support_website_text

%br
2 changes: 2 additions & 0 deletions config/settings.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ docs:
:infra_provider: http://manageiq.org/docs/reference/latest/doc-Managing_Providers/miq/#infrastructure_providers
:network_provider: http://manageiq.org/docs/reference/latest/doc-Managing_Providers/miq/#network_providers
:physical_infra_provider: http://manageiq.org/docs/reference/latest/doc-Managing_Providers/miq/#infrastructure_providers
:product_support_website: http://www.manageiq.org
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be in the core repo?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the whole docs section feels like it belongs in core.

Copy link
Member

@Fryguy Fryguy Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There are a number of things wrong with this, but

  • Yes, I agree this entire section should live in core.
  • All of the links should be https
  • provider type links are fine, but then "ansible" (which should really be ansible_tower) is a provider specific link, which should be pluggable, but I'd argue that it shouldn't exist at all since we don't have any of the other providers
  • top-level docs is a string, but :ui is a symbol

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point of the change was to use Settings instead of ui_lookup. Anything else is optional :).
(Note it didn't actually change any of the links, feel free to update them if you know what they should be.)

As for ansible - maybe it should really be automation. It's used in the "no records" screen in /automation_manager/explorer (Automation > Ansible Tower > Explorer)

Copy link
Contributor

Choose a reason for hiding this comment

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

(Except for product_support_*, none of the other keys are used anywhere outside the UI.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having bit of a problem understanding why should this config section live in core?

Copy link
Contributor

@himdel himdel Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IMO it doesn't make sense to move these to the core without a core mechanism for "get me docs for this model kind of provider" (infra/cloud/automation...),
and the UI won't consume it over the API before we have an API endpoint for list views with all the info the UI needs. (Should we plan for pluggable provider kinds?)

What am I missing? :)

Copy link
Member

Choose a reason for hiding this comment

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

I opened https://github.com/ManageIQ/manageiq/pull/20010/files and #6793 to move the settings.

Copy link
Member

Choose a reason for hiding this comment

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

ManageIQ/manageiq-api#788 is how I got here. Which struck me as odd that UI settings are being used for the API.

Copy link
Member

@Fryguy Fryguy Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think the point of the change was to use Settings instead of ui_lookup

Yeah, we're not disputing the usage of Settings, we're disputing where the Settings should live.

Anything else is optional :).

Totally agree...My list was not for this PR but for a follow up.

and the UI won't consume it over the API before we have an API endpoint for list views with all the info the UI needs. (Should we plan for pluggable provider kinds?)

Settings are already available via the API, and providers can already bring pluggable settings, though in this case they are actually provider-type which is a core construct anyway, so it doesn't matter.

:product_support_website_text: ManageIQ.org