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

Add flag for init of defaults in fields #18061

Merged
merged 1 commit into from
Oct 15, 2018
Merged

Add flag for init of defaults in fields #18061

merged 1 commit into from
Oct 15, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Oct 4, 2018 •
edited
Loading

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

At this point if a service dialog gets ordered via the API, it will not honor defaults on static fields. To remedy this, this adds a flag to the call to also let us call load_values_into_fields(values, false), thus honoring the default values. This builds on the recent changes to #17844

Related:

ManageIQ/manageiq-api#485

@d-m-u d-m-u mentioned this pull request Oct 4, 2018
Merged
@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 4, 2018

@eclarizio can you 👀 please?

eclarizio
eclarizio suggested changes Oct 4, 2018
View reviewed changes
Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

This option is going to need to be propagated through this, I think. https://github.com/ManageIQ/manageiq/blob/master/app/models/service_template.rb#L426

@d-m-u d-m-u changed the title Add flag for init of defaults in fields [WIP] Add flag for init of defaults in fields Oct 4, 2018
@miq-bot miq-bot added the wip label Oct 5, 2018
eclarizio
eclarizio reviewed Oct 5, 2018
View reviewed changes
Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

Code changes look fine, the specs should be easy to add for resource_action_workflow since we already have a section where the different options are being enabled. service_template spec might be a bit more involved to add a spec, but I'm sure you can do it 👍

@d-m-u d-m-u force-pushed the bz1635673 branch 4 times, most recently from 148893f to 73e830d Compare October 8, 2018 15:55
@d-m-u d-m-u changed the title [WIP] Add flag for init of defaults in fields Add flag for init of defaults in fields Oct 8, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 8, 2018

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Oct 8, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 9, 2018

Hey @eclarizio can you please review again?

eclarizio
eclarizio approved these changes Oct 9, 2018
View reviewed changes
Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

I think this side is good to go. The API side can be merged after this to take advantage of the new logic

👍

@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 9, 2018

@miq-bot add_reviewer @tinaafitz

@miq-bot miq-bot requested a review from tinaafitz October 9, 2018 19:28
@JPrause
Copy link
Member

JPrause commented Oct 9, 2018

@miq-bot add_label blocker

@miq-bot miq-bot added the blocker label Oct 9, 2018
@JPrause
Copy link
Member

JPrause commented Oct 9, 2018

@d-m-u if this will be able to be backported, please add the gaprindashvili/yes and hammer/yes labels

@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 9, 2018

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Oct 9, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 9, 2018 •
edited
Loading

@miq-bot add_label gaprindashvili/yes, hammer/yes

@miq-bot

This comment has been minimized.

Sign in to view
@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 10, 2018

@miq-bot assign @gmcculloug

@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 10, 2018

@bdunne can you please 👀 for me?

bdunne
bdunne reviewed Oct 11, 2018
View reviewed changes
app/models/service_template.rb Outdated
@@ -427,7 +427,8 @@ def provision_workflow(user, dialog_options = nil, request_options = {})
:target => self,
:initiator => request_options[:initiator],
:submit_workflow => request_options[:submit_workflow],
:provision_workflow => request_options[:provision_workflow]
:provision_workflow => request_options[:provision_workflow],
Copy link
Member

Choose a reason for hiding this comment

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

ra_options = request_options.slice(:initiator, :init_defaults, ...).merge(:target => self)?

@JPrause
Copy link
Member

JPrause commented Oct 12, 2018

@d-m-u is this ready for merge, if yes, @gmcculloug can you merge.

@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 12, 2018

This, and the related ManageIQ/manageiq-api#485 should be both good to go

gmcculloug
gmcculloug reviewed Oct 15, 2018
View reviewed changes
config/settings.yml Outdated
@@ -980,6 +980,7 @@
:name:
:maindb: ExtManagementSystem
:container_deployment_wizard: false
:run_automate_methods_on_service_catalog_api_submit: false
Copy link
Member

Choose a reason for hiding this comment

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

The only thing I would suggest is dropping the catalog from the name. Technically it is a service_template but I think run_automate_methods_on_service_api_submit is enough.

@miq-bot
Copy link
Member

miq-bot commented Oct 15, 2018

Checked commit d-m-u@4c4601d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

@gmcculloug gmcculloug merged commit 2ab08ae into ManageIQ:master Oct 15, 2018
@gmcculloug gmcculloug added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 15, 2018
simaishi pushed a commit that referenced this pull request Oct 15, 2018
@gmcculloug @simaishi
Merge pull request #18061 from d-m-u/bz1635673
4b30495 
Add flag for init of defaults in fields

(cherry picked from commit 2ab08ae)

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

Hammer backport details:

$ git log -1
commit 4b30495e2085c2e8d721d22bd5d87ed592a6662d
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Oct 15 09:39:38 2018 -0400

    Merge pull request #18061 from d-m-u/bz1635673
    
    Add flag for init of defaults in fields
    
    (cherry picked from commit 2ab08ae6623053f50342dcecdbd3f5f87eadd04f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1635673

@d-m-u d-m-u deleted the bz1635673 branch October 15, 2018 13:56
@simaishi
Copy link
Contributor

@d-m-u service_template_spec.rb conflicts on backport to Gaprindashvili branch.... Can you create a separaete PR?

@d-m-u d-m-u mentioned this pull request Oct 15, 2018
Merged
@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 15, 2018 •
edited
Loading

@simaishi sure, it's here: #18099

@simaishi
Copy link
Contributor

Backported to Gaprindashvili via #18099

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

@bdunne bdunne bdunne left review comments

@gmcculloug gmcculloug gmcculloug left review comments

@eclarizio eclarizio eclarizio approved these changes

@tinaafitz tinaafitz Awaiting requested review from tinaafitz

Assignees

@gmcculloug gmcculloug

Labels
blocker bug gaprindashvili/backported hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@d-m-u @JPrause @miq-bot @simaishi @eclarizio @bdunne @gmcculloug