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

Add Ansible related inputs to GOD haml #5166

Merged
merged 8 commits into from
Jun 21, 2019

Conversation

ZitaNemeckova
Copy link
Contributor

@ZitaNemeckova ZitaNemeckova commented Jan 17, 2019

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650559

Go to Automation -> Automate -> Generic Object -> create or edit a button -> select Button type to be Ansible Playbook

Depends on ManageIQ/manageiq#18379

Before:
image

After:
image

Ansible Catalog Item is required!

@miq-bot add_label wip, bug, hammer/yes, gaprindashvili/yes, generic objects, automation/
automate

@miq-bot
Copy link
Member

miq-bot commented Jan 17, 2019

@ZitaNemeckova Cannot apply the following label because they are not recognized: automation/

@miq-bot miq-bot changed the title Add Ansible related inputs to GOD haml [WIP] Add Ansible related inputs to GOD haml Jan 17, 2019
@ZitaNemeckova
Copy link
Contributor Author

@miq-bot add_label pending core

@ZitaNemeckova
Copy link
Contributor Author

@miq-bot remove_label wip, pending core

@miq-bot miq-bot changed the title [WIP] Add Ansible related inputs to GOD haml Add Ansible related inputs to GOD haml Feb 13, 2019
if (vm.customButtonRecordId) {
vm.newRecord = false;
miqService.sparkleOn();
dataPromise = API.get('/api/custom_buttons/' + vm.customButtonRecordId + '?attributes=resource_action,uri_attributes')
Copy link
Contributor

@himdel himdel Feb 22, 2019

Choose a reason for hiding this comment

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

this won't work - you're setting dataPromise after you use it in $q.all, so we never wait for this one.

But.. looks like no part of this request depends on anything from the options request, right? (I mean to make the request, not to process the response.)

So, maybe something like...

var optionsPromise = API.options('/api/custom_buttons')
  .then(function(response) {
    _.forEach(response.data.custom_button_types, function(name, id) {
      vm.button_types.push({id: id, name: name});
    });
  });

if (vm.customButtonRecordId) {
  vm.newRecord = false;
  //miqService.sparkleOn(); - see second coment

  var dataPromise = API.get('/api/custom_buttons/' + vm.customButtonRecordId + '?attributes=resource_action,uri_attributes');
    .then(getCustomButtonFormData) // getCustomButtonFormData already waits for optionsPromise inside
    .catch(miqService.handleFailure);
} else {
  vm.newRecord = true;
  vm.modelCopy = angular.copy( vm.customButtonModel );
}

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, since we're always waiting for some responses,
we should probably sparkleOn every time (not only with id), and sparkleOff in the big $q.all block.

@@ -258,8 +283,14 @@ function mainCustomButtonFormController(API, miqService, $q, $http) {
vm.customButtonModel.attribute_names,
vm.customButtonModel.attribute_values);

if(vm.customButtonModel.uri_attributes.hosts == undefined || vm.customButtonModel.uri_attributes.hosts === "localhost" || vm.customButtonModel.uri_attributes.hosts == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing spaces after if, and before { on line 288

also, no point testing both vm.customButtonModel.uri_attributes.hosts == undefined and vm.customButtonModel.uri_attributes.hosts == null. Those are exactly the same thing without ===.

(And this should really be just if (!vm.customButtonModel.uri_attributes.hosts || vm.customButtonModel.uri_attributes.hosts === 'localhost') .. comparing against null or undefined is usually not what you want :).

@@ -243,7 +267,8 @@ function mainCustomButtonFormController(API, miqService, $q, $http) {

vm.genericObjectDefinitionRecordId = response.applies_to_id;

optionsPromise.then(function() {
vm.customButtonModel.uri_attributes = response.uri_attributes;

if (vm.customButtonModel.current_visibility === 'role') {
Copy link
Contributor

Choose a reason for hiding this comment

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

bad indent for this whole block? (246..262 in original)

@ZitaNemeckova
Copy link
Contributor Author

@miq-bot add_label changelog/yes

@ZitaNemeckova
Copy link
Contributor Author

@himdel please re-review, thanks :)

@himdel
Copy link
Contributor

himdel commented May 3, 2019

Sure, but fix the miq-bot error please? :)

%label.control-label.col-md-2
= _("Inventory")
.col-md-8
%input{:type => "radio", :id => "inventory_localhost", :value => "localhost", 'ng-model' => 'vm.inventory'}
Copy link
Contributor

@himdel himdel May 3, 2019

Choose a reason for hiding this comment

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

We now have those 3 options in:

app/views/static/generic_object/main_custom_button_form.html.haml
44:      = label_tag('inventory_localhost', _("Localhost"), "title" => _('Run on localhost'))
47:      = label_tag('inventory_event_target', _("Target Machine"), "title" => _('Run on the target of the Policy Event'))
50:      = label_tag(_("Specific Hosts"))

app/views/shared/_playbook_options.html.haml
24:    = label_tag('inventory_localhost', _("Localhost"), "title" => _('Run on localhost'))
29:    = label_tag('inventory_event_target', _("Target Machine"), "title" => _('Run on the target of the Policy Event'))
34:    = label_tag('inventory_manual', _("Specific Hosts"), "title" => _('Enter a comma separated list of IP or DNS names'))

app/views/miq_policy/_action_options.html.haml
341:          = label_tag('inventory_localhost', _("Localhost"), "title" => _('Run on localhost'))
347:          = label_tag('inventory_event_target', _("Target Machine"), "title" => _('Run on the target of the Policy Event'))
353:          = label_tag('inventory_manual', _("Specific Hosts"), "title" => _('Enter a comma separated list of IP or DNS names'))

Time for a partial, I think ;)

EDIT: never mind, the other 2 are non-angular ..

Copy link
Contributor

@himdel himdel May 3, 2019

Choose a reason for hiding this comment

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

Also, the inputs are missing name="inventory". Otherwise, you have 3 unrelated radio buttons, instead of one radio button group (it probably works anyway because they do share the same ng-model).

@miq-bot
Copy link
Member

miq-bot commented May 30, 2019

Checked commits ZitaNemeckova/manageiq-ui-classic@38e6ad7~...285c611 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

**

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

Remove unused id
Fix bad ids and bad intent
Move dataPromise out of OptionsPromise because they don't depend on each other
@ZitaNemeckova
Copy link
Contributor Author

@himdel This is ready for another review :)

@ZitaNemeckova
Copy link
Contributor Author

@romanblanco could you test/have a look at this one, please?

@romanblanco
Copy link
Member

@miq-bot assign @romanblanco

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Tested, works 👍

@himdel himdel added this to the Sprint 114 Ending Jun 24, 2019 milestone Jun 21, 2019
@himdel himdel merged commit 655f798 into ManageIQ:master Jun 21, 2019
@ZitaNemeckova ZitaNemeckova deleted the add_ansible_to_GOD branch June 21, 2019 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants