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

Added input field for max playbook_ttl value #1742

Merged
merged 2 commits into from
Jul 27, 2017
Merged

Added input field for max playbook_ttl value #1742

merged 2 commits into from
Jul 27, 2017

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented Jul 24, 2017 •
edited by dclarizio
Loading

Added new input field on both Provisioning & retirement tabs to allow user to enter maximum playbook_ttl numeric value.

https://www.pivotaltracker.com/story/show/147956383
https://bugzilla.redhat.com/show_bug.cgi?id=1459735

screenshots:
max_ttl_numeric_value

max_ttl_validation

catalog_item_summary

@tinaafitz please test.

@h-kataria h-kataria force-pushed the allow_max_ttl_for_ansible_items branch from baca664 to b898178 Compare July 24, 2017 16:52
@dclarizio dclarizio requested a review from AparnaKarve July 24, 2017 17:07
@dclarizio dclarizio self-assigned this Jul 24, 2017
AparnaKarve
AparnaKarve reviewed Jul 24, 2017
View reviewed changes
app/assets/javascripts/controllers/catalog/catalog_item_form_controller.js Outdated
@@ -247,6 +251,8 @@ ManageIQ.angular.app.controller('catalogItemFormController', ['$scope', 'catalog
}
}
}
if (vm.catalogItemModel.provisioning_playbook_ttl !== undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the value be undefined for vm.catalogItemModel.provisioning_playbook_ttl?
I guess that depends on the .rb code below? -

playbook_details[:provisioning][:playbook_ttl] = provision[:playbook_ttl]

If you don't expect the value to be undefined, then you can skip this conditional.

(Also preferred syntax for if per the styling guide is -

if (condition) {
  // code below
  ...
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AparnaKarve it can be undefined when editing existing records, and when the value for the textbox is not provided

Copy link
Contributor

Choose a reason for hiding this comment

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

@h-kataria Ok, thanks.

AparnaKarve
AparnaKarve reviewed Jul 24, 2017
View reviewed changes
app/assets/javascripts/controllers/catalog/catalog_item_form_controller.js Outdated
@@ -271,6 +277,8 @@ ManageIQ.angular.app.controller('catalogItemFormController', ['$scope', 'catalog
retirement['credential_id'] = configData.retirement_machine_credential_id;
}
if (vm.catalogItemModel.retirement_playbook_id !== undefined && configData.retirement_playbook_id !== '') {
if (vm.catalogItemModel.retirement_playbook_ttl !== undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

Above comment is applicable here as well.

AparnaKarve
AparnaKarve reviewed Jul 24, 2017
View reviewed changes
app/assets/javascripts/controllers/catalog/catalog_item_form_controller.js Outdated
@@ -123,6 +125,7 @@ ManageIQ.angular.app.controller('catalogItemFormController', ['$scope', 'catalog
vm.catalogItemModel.provisioning_machine_credential_id = configData.provision.credential_id;
vm.catalogItemModel.provisioning_network_credential_id = configData.provision.network_credential_id;
vm.catalogItemModel.provisioning_cloud_credential_id = setIfDefined(configData.provision.cloud_credential_id);
vm.catalogItemModel.provisioning_playbook_ttl = configData.provision.playbook_ttl;
vm.catalogItemModel.provisioning_inventory = configData.provision.hosts;
vm.catalogItemModel.provisioning_dialog_existing = configData.provision.dialog_id ? "existing" : "create";
vm.catalogItemModel.provisioning_dialog_id = configData.provision.dialog_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the TODO refactoring items is to use Object.assign instead of copying the items individually.
For e.g. -

Object.assign(vm.catalogItemModel, configData.provision);

(Just a FYI, and can be done later in a separate refactoring PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AparnaKarve i will address that in a follow up PR.

@h-kataria h-kataria force-pushed the allow_max_ttl_for_ansible_items branch from b898178 to 2039b84 Compare July 24, 2017 18:27
@h-kataria
Added input field for max playbook_ttl value
152605c 
Added new input field on both Provisioning & retirement tabs to allow user to enter maximum playbook_ttl numeric value.

https://www.pivotaltracker.com/story/show/147956383
@h-kataria h-kataria force-pushed the allow_max_ttl_for_ansible_items branch from 2039b84 to 152605c Compare July 25, 2017 20:52
@h-kataria h-kataria force-pushed the allow_max_ttl_for_ansible_items branch from 1066c8d to fe47123 Compare July 26, 2017 04:05
@miq-bot
Copy link
Member

miq-bot commented Jul 26, 2017

Checked commits h-kataria/manageiq-ui-classic@152605c~...fe47123 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

@martinpovolny martinpovolny added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 27, 2017
@martinpovolny martinpovolny merged commit 41cef52 into ManageIQ:master Jul 27, 2017
@h-kataria h-kataria deleted the allow_max_ttl_for_ansible_items branch July 27, 2017 13:49
@dclarizio dclarizio added fine/yes and removed fine/no labels Aug 1, 2017
simaishi pushed a commit that referenced this pull request Aug 9, 2017
@martinpovolny @simaishi
Merge pull request #1742 from h-kataria/allow_max_ttl_for_ansible_items
e5cc508 
Added input field for max playbook_ttl value
(cherry picked from commit 41cef52)

https://bugzilla.redhat.com/show_bug.cgi?id=1479407
@simaishi
Copy link
Contributor

simaishi commented Aug 9, 2017

Fine backport details:

$ git log -1
commit e5cc5081db97650206cee04b6f445a7c1d749025
Author: Martin Povolny <[email protected]>
Date:   Thu Jul 27 09:46:47 2017 +0200

    Merge pull request #1742 from h-kataria/allow_max_ttl_for_ansible_items
    
    Added input field for max playbook_ttl value
    (cherry picked from commit 41cef52c5f0c0bf2d4322826387529a04083d810)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1479407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AparnaKarve AparnaKarve AparnaKarve left review comments

Assignees

@martinpovolny martinpovolny

Labels
enhancement fine/backported services
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@h-kataria @miq-bot @simaishi @AparnaKarve @martinpovolny @dclarizio