-
Notifications
You must be signed in to change notification settings - Fork 356
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
Use product settings for docs & links #6718
Use product settings for docs & links #6718
Conversation
Checked commit mzazrivec@b61ee1e with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint |
LGTM, removes the last uses of (We still have..
) |
@@ -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 |
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.
Shouldn't these be in the core repo?
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.
Actually, the whole docs
section feels like it belongs in core.
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.
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
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.
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)
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.
(Except for product_support_*
, none of the other keys are used anywhere outside the UI.)
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.
I'm having bit of a problem understanding why should this config section live in core?
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.
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? :)
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.
I opened https://github.com/ManageIQ/manageiq/pull/20010/files and #6793 to move the settings.
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.
ManageIQ/manageiq-api#788 is how I got here. Which struck me as odd that UI settings are being used for the API.
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.
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.
Removed |
Fixes #6083