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.

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

More automate methods #5295

Merged
merged 26 commits into from Apr 29, 2019
Merged

Conversation

martinpovolny
Copy link

@martinpovolny martinpovolny commented Mar 4, 2019

Automate --> Automate --> Explorer --> Select some "Methods" in the left tree --> Swith to "Methods" Tab in the content area --> Click "Add a new Method" in the toolbar.

Closes #5288
Closes #5287

Requires ManageIQ/manageiq#18160

Problems:

  • The form is used on one other place. Make sure it still works when this is done. (it does)
  • editing does not work at all from the "detail view" (fixed)
  • need some "loading" state for the list of playbooks (the wheel?)

Notes

Forms are partly server side and partly Angular

Entry for the server-side is app/views/miq_ae_class/_method_form.html.haml
Entry for the angular part is app/views/miq_ae_class/_angular_method_form.html.haml

Choice between server-side and angular is done in app/controllers/miq_ae_class_controller.rb, based on location (e.g. @ae_method.location). The value is what is presented to the user asthe "Main Info / Type" from the screenshots.

I am introducing helper written_in_angular playbook_style_location? to limit the spread of similar conditions....

data for the Angular controller come from #method_form_fields which currently only works for location = "playbook" -- need to extend that.

Angular controller (javascript) lives in app/assets/javascripts/controllers/miq_ae_class/ae_method_form_controller.js

  • list of AnsibleTower providers ... Provider.where(...)
  • list of conf. scripts on that provider: ManageIQ::Providers::AnsibleTower::AutomationManager::ConfigurationScript ....?
  • list of conf. workflows ManageIQ::Providers::AnsibleTower::AutomationManager::ConfigurationWorkflow

relation to provider via .manager_id ?

I found fixed some missing feature checking and also this:

all-2x-2019-03-05_13-41

This shows that the Angular part of the editor fires each API request 2x. I will need to investigate and fix this too. (fixed)

The commit "Automate method: don't call formOptions() 2x. 4 extra API calls." fixes that. Thx @himdel

@martinpovolny

This comment has been minimized.

@martinpovolny

This comment has been minimized.

@martinpovolny

This comment has been minimized.

@martinpovolny

This comment has been minimized.

@martinpovolny

This comment has been minimized.

@martinpovolny

This comment has been minimized.

@martinpovolny martinpovolny force-pushed the more_automate_methods branch 2 times, most recently from 37d1b39 to f8e096a Compare March 8, 2019 13:54
@martinpovolny

This comment has been minimized.

@martinpovolny
Copy link
Author

martinpovolny commented Mar 8, 2019

Further missing (not in the issues but needed):

  • Update the View screen. (done)
  • Add icons to the tree.

missing-automate-method

I assume these parts are also needed, @tinaafitz, @Flu.

Any idea what icons should be used for the 2 new types?

@tinaafitz
Copy link
Member

Hi @martinpovolny It looks great. Thanks for all of your hard work.
I discussed icons with @epwinchell a while ago, and I think he mentioned that we could use generic icons for workflows and templates.
@epwinchell Did I get that right? I know you and I discussed the possibility of using Ansible specific icons, but I think generic icons would work well.

@epwinchell
Copy link
Contributor

I discussed icons with @epwinchell a while ago, and I think he mentioned that we could use generic icons for workflows and templates.

I think "pficon-template" for template and "ff-load-balancer" would probably be fine for workflow:

Screen Shot 2019-03-08 at 2 54 20 PM

Ok, @martinpovolny ?

@martinpovolny
Copy link
Author

This hopefully adds the iconc ManageIQ/manageiq-decorators#3
(TODO: so far I saw one, do not have data for the Workflows)

@martinpovolny
Copy link
Author

@lfu : can you, please, help me with testing this? You are the one who knows what this should be doing... Thx!

@lfu
Copy link
Member

lfu commented Mar 15, 2019

Screen Shot 2019-03-15 at 12 21 49

Screen Shot 2019-03-15 at 12 13 14

@martinpovolny
Copy link
Author

@h-kataria : I see that the form is used on one more place. Through here:

lucifer - [~/Projects/manageiq-ui-classic] (more_automate_methods)$ grep -r st_angular_form app/
app/controllers/catalog_controller.rb:        page.replace("form_div", :partial => "st_angular_form")
app/controllers/catalog_controller.rb:                r[:partial => ansible_playbook? ? "st_angular_form" : "st_form"]     
app/views/catalog/_st_angular_form.html.haml:    = render :partial => "layouts/angular/multi_tab_ansible_form_options",

So that's somewhere in services.

Seems you are the one who created that part. Can you, please, tell me, how I reach that code? I'd like to make sure that I am not breaking that area by my changes in this area.

@himdel
Copy link
Contributor

himdel commented Mar 19, 2019

So that's somewhere in services.

Services > Catalogs, accordion Catalog Items,
toolbar Configure - Add a New Catalog Item
pick Ansible Playbook as the type

items

@martinpovolny martinpovolny changed the title [WIP] More automate methods More automate methods Mar 19, 2019
@miq-bot miq-bot removed the wip label Mar 19, 2019
@martinpovolny
Copy link
Author

@h-kataria : Seems you are one of the people who where the last ones to touch this area. Would you, please, find some time to check this PR?

@martinpovolny
Copy link
Author

Added a commit adressing this comment: #5287 (comment)

@miq-bot
Copy link
Member

miq-bot commented Apr 4, 2019

This pull request is not mergeable. Please rebase and repush.

@martinpovolny martinpovolny force-pushed the more_automate_methods branch 2 times, most recently from 2beb427 to 232b8af Compare April 29, 2019 13:35
@martinpovolny
Copy link
Author

martinpovolny commented Apr 29, 2019

@h-kataria : Thank you for your testing!

Please, check once again. I did changes to fix the crashes that you encountered but I am not sure I reproduced the "... details screen it does not show ...." properly. Anyway I cannot get it NOT display the provider now.

Regarding the second issue, I did fix the crashes. However as you write:

"It appears if i add a new method of above types, and do not select Job/Workflow template while adding them, it does not save Provider selection."

That is correct, because I am not saving the provider at all, just the template ID. Therefor w/o a template ID I have nothing to save. However it did not have to crash. Now it does not, so, please, retest and see the 2 last commits.

@miq-bot
Copy link
Member

miq-bot commented Apr 29, 2019

Checked commits martinpovolny/manageiq-ui-classic@ea9cce8~...d7e78d3 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
9 files checked, 2 offenses detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

app/controllers/miq_ae_class_controller.rb

@h-kataria
Copy link
Contributor

@martinpovolny it is still not saving Provider info when adding/editing methods when Job/Workflow template is not selected, is that an expected behaviour? Wondering if selecting template should be a required field, @tinaafitz cc

@h-kataria
Copy link
Contributor

Tested adding new method types in the PR, working fine now, If we decide that Workflow/Job template field should be a required field, validation should be added for that in backend code. UI changes LGTM

@h-kataria h-kataria self-assigned this Apr 29, 2019
@h-kataria h-kataria added this to the Sprint 110 Ending Apr 29, 2019 milestone Apr 29, 2019
@h-kataria h-kataria merged commit 5fcd0f6 into ManageIQ:master Apr 29, 2019
@martinpovolny
Copy link
Author

martinpovolny commented Apr 29, 2019

@martinpovolny it is still not saving Provider info when adding/editing methods when Job/Workflow template is not selected, is that an expected behaviour?

yes

Wondering if selecting template should be a required field, @tinaafitz cc

I think it should be a validation on the model level. Adding it at model level will also work through the API. On the other side, if I add a validation here that would be on the UI side in a controller and that is IMO new tech debt.

@martinpovolny
Copy link
Author

@kbrock
Copy link
Member

kbrock commented Sep 12, 2019

@martinpovolny I'm now seeing:

app/views/layouts/angular/_ansible_form_options_angular.html.haml:133:
warning: key "ng-if" is duplicated and overwritten on line 135

@ZitaNemeckova
Copy link
Contributor

@kbrock Will be fixed in #6382

@ZitaNemeckova ZitaNemeckova mentioned this pull request Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment