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

Jump to bottom

Support for opening URL from the UI through automate. #328

Merged
merged 1 commit into from
Jul 11, 2019
Merged

Support for opening URL from the UI through automate. #328

merged 1 commit into from
Jul 11, 2019

Conversation

martinpovolny
Copy link
Member

@martinpovolny martinpovolny commented Jun 11, 2019 •
edited by gmcculloug
Loading

@miq-bot miq-bot added the wip label Jun 11, 2019
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2611

  • 9 of 10 (90.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 84.926%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/miq_automation_engine/service_models/mixins/miq_ae_saved_url_mixin.rb 3 4 75.0%
Totals Coverage Status
Change from base Build 2593: 0.009%
Covered Lines: 4896
Relevant Lines: 5765
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 11, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 2732

  • 17 of 18 (94.44%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 84.969%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/miq_automation_engine/service_models/mixins/miq_ae_external_url_mixin.rb 3 4 75.0%
Totals Coverage Status
Change from base Build 2728: 0.05%
Covered Lines: 4918
Relevant Lines: 5788
💛 - Coveralls

@gmcculloug
Copy link
Member

@martinpovolny Please include the BZ in the PR description. Thanks.

@martinpovolny martinpovolny mentioned this pull request Jun 12, 2019
Merged
gmcculloug
gmcculloug reviewed Jun 17, 2019
View reviewed changes
lib/miq_automation_engine/service_models/mixins/miq_ae_saved_url_mixin.rb Outdated
@@ -0,0 +1,7 @@
module MiqAeSavedUrlMixin
extend ActiveSupport::Concern
def saved_url=(url)
Copy link
Member

Choose a reason for hiding this comment

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

@martinpovolny If we are specifically using this for the custom button URL it would be better if that name reflected that. The method name exposed here does not have to match the backend model method, but that usually makes things easier so we should seriously consider naming at each level.

This was referenced Jul 1, 2019
Merged
Merged
@gmcculloug gmcculloug self-assigned this Jul 8, 2019
@gmcculloug
Copy link
Member

@martinpovolny Is this ready to merge?

@martinpovolny
Copy link
Member Author

@gmcculloug : I need to add the mixin for the provider classes too. Looking into that now.

@martinpovolny
Copy link
Member Author

martinpovolny commented Jul 10, 2019 •
edited
Loading

I wonder if I need to add the mixin to MiqAeServiceManageIQ_Providers_CloudManager or into MiqAeServiceManageIQ_Providers_BaseManager and if the latter then where does the class live?

@gmcculloug
Copy link
Member

Based on the BUTTON_CLASSES defined here:
https://github.com/ManageIQ/manageiq/blob/master/app/models/custom_button.rb#L41

It would get added here to support Providers:
https://github.com/ManageIQ/manageiq-automation_engine/blob/master/lib/miq_automation_engine/service_models/miq_ae_service_ext_management_system.rb

@martinpovolny martinpovolny changed the title [WIP] Support for opening URL from the UI through automate. Support for opening URL from the UI through automate. Jul 10, 2019
@miq-bot miq-bot removed the wip label Jul 10, 2019
@miq-bot
Copy link
Member

miq-bot commented Jul 10, 2019

Checked commit martinpovolny@364c8cc with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 🍰

gmcculloug
gmcculloug reviewed Jul 10, 2019
View reviewed changes
lib/miq_automation_engine/service_models/miq_ae_service_vm.rb
@@ -1,5 +1,8 @@
module MiqAeMethodService
class MiqAeServiceVm < MiqAeServiceVmOrTemplate
require_relative "mixins/miq_ae_external_url_mixin"
include MiqAeExternalUrlMixin
Copy link
Member

Choose a reason for hiding this comment

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

@martinpovolny VM was not in the list provided in https://bugzilla.redhat.com/show_bug.cgi?id=1550002#c6.

If a user can set both remote_console_url= and external_url= what happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gmcculloug : in the UI I want to look for the external_url first and then as a fallback for the remote_console_url (SystemConsole entity). So that backwards compatibility is kept.

Like this:
https://github.com/ManageIQ/manageiq-ui-classic/pull/5688/files#diff-65512d4378cfa992fb57fbceaaf14c59R262

I do not think that we should do things differently for different entities. So moving forward I'd like to deprecate the remote_console_url.

Does that make sense to you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively I'd change the remote_console_url method to assign to the same place as external_url and add a migration to remove the column. But again, it can be done as a next step.

Copy link
Member

Choose a reason for hiding this comment

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

My understand is the remote_console_url has a lot more support today for the specific task of setting up a proxy to support VM consoles, but it can also support what external_url is trying to do. As long as a combined field can support the two use-cases then I'm good.

Copy link
Member Author

@martinpovolny martinpovolny Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

When I added this it was used for this single purpose.

The entity, that it writes to, the SystemConsole, that is used for the proxies.

But not this particular column or the particular method remote_console_url=

Copy link
Member

Choose a reason for hiding this comment

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

Got it, let's create an issue to track the follow items, otherwise I'm good with this change. 👍

@gmcculloug gmcculloug merged commit b1c07a1 into ManageIQ:master Jul 11, 2019
@gmcculloug gmcculloug added this to the Sprint 116 Ending Jul 22, 2019 milestone Jul 11, 2019
@martinpovolny martinpovolny mentioned this pull request Jul 11, 2019
Open
@martinpovolny martinpovolny mentioned this pull request Aug 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

Assignees

@gmcculloug gmcculloug

Labels
changelog/yes enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinpovolny @coveralls @gmcculloug @miq-bot