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 service_vars to pass variables between catalog items in a bundle #338

Merged
merged 1 commit into from Jul 23, 2019
Merged

Add service_vars to pass variables between catalog items in a bundle #338

merged 1 commit into from Jul 23, 2019

Conversation

lfu
Copy link
Member

@lfu lfu commented Jul 15, 2019

@coveralls
Copy link

coveralls commented Jul 15, 2019 •
edited

Pull Request Test Coverage Report for Build 2784

  • 27 of 29 (93.1%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 85.017%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service.rb 14 16 87.5%
Totals Coverage Status
Change from base Build 2742: 0.04%
Covered Lines: 4948
Relevant Lines: 5820
💛 - Coveralls

@lfu lfu force-pushed the service_vars_1678136 branch 2 times, most recently from 59e6c1c to bcc9298 Compare July 17, 2019 20:04
tinaafitz
tinaafitz approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@lfu Looks good.

@tinaafitz
Copy link
Member

@bdunne @billfitzgerald0120 Please review.

bdunne
bdunne reviewed Jul 17, 2019
View reviewed changes
lib/miq_automation_engine/service_models/miq_ae_service_service.rb Outdated

def set_service_vars_option(key, value)
ar_method do
@object.options[:service_vars] ||= HashWithIndifferentAccess.new
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use HashWithIndifferentAccess? I know we've had problems deserializing after upgrades before. cc @jrafanie

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how a HWIA could be a problem. It does mask inconsistent "hash all the way down" interfaces but should be ok if we can't expect for client/consumers to use a consistent hash interface (string vs. symbols). 🤷‍♂

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, the namespace of HWIA changed in a Rails version upgrade and we had to create an alias or something in order to deserialize them, but I can't find it right now so maybe I'm thinking of something else.

bdunne
bdunne reviewed Jul 17, 2019
View reviewed changes
spec/miq_ae_service_spec.rb Outdated Show resolved Hide resolved
bdunne
bdunne reviewed Jul 18, 2019
View reviewed changes
lib/miq_automation_engine/service_models/miq_ae_service_service.rb Outdated
@@ -48,6 +48,32 @@ def set_dialog_option(key, value)
end
end

def service_vars_options
@object.options[:service_vars] || HashWithIndifferentAccess.new
Copy link
Member

Choose a reason for hiding this comment

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

Should this be ||=?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so.

bdunne
bdunne reviewed Jul 18, 2019
View reviewed changes
lib/miq_automation_engine/service_models/miq_ae_service_service.rb Outdated

def set_service_vars_option(key, value)
ar_method do
@object.options[:service_vars] ||= HashWithIndifferentAccess.new
Copy link
Member

Choose a reason for hiding this comment

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

Can the service_vars_options method be reused here rather than duplicating it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the set method. The other one is the get method to retrieve data from DB.

Copy link
Member

Choose a reason for hiding this comment

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

I think @bdunne is correct that this should reuse the service_vars_options method and that the service_vars_options method should use ||= as mentioned above. That would reduce the places that have to know the options hash key and the HashWithIndifferentAccess logic.

gmcculloug
gmcculloug reviewed Jul 22, 2019
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service.rb Outdated
@@ -92,6 +92,37 @@ def ansible_stats_vars
MiqAeEngine::MiqAeAnsibleMethodBase.ansible_stats_from_hash(@persist_state_hash)
end

def service_object
Copy link
Member

Choose a reason for hiding this comment

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

Is this only meant for internal use? If so it can be moved to the private section of the file.

lib/miq_automation_engine/service_models/miq_ae_service_service.rb Outdated

def set_service_vars_option(key, value)
ar_method do
@object.options[:service_vars] ||= HashWithIndifferentAccess.new
Copy link
Member

Choose a reason for hiding this comment

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

I think @bdunne is correct that this should reuse the service_vars_options method and that the service_vars_options method should use ||= as mentioned above. That would reduce the places that have to know the options hash key and the HashWithIndifferentAccess logic.

lib/miq_automation_engine/service_models/miq_ae_service_service.rb Outdated
end

def delete_service_vars_option(key)
return unless @object.options[:service_vars]&.key?(key)
Copy link
Member

Choose a reason for hiding this comment

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

This could use service_vars_options as well.

@lfu lfu force-pushed the service_vars_1678136 branch 2 times, most recently from 74b05ba to b459521 Compare July 22, 2019 21:47
@miq-bot
Copy link
Member

miq-bot commented Jul 22, 2019

Checked commit lfu@a8eb5a3 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

@gmcculloug gmcculloug merged commit 09c8bbd into ManageIQ:master Jul 23, 2019
@gmcculloug gmcculloug self-assigned this Jul 23, 2019
@gmcculloug gmcculloug added this to the Sprint 116 Ending Jul 22, 2019 milestone Jul 23, 2019
@lfu
Copy link
Member Author

lfu commented Jul 23, 2019

@miq-bot add_label changelog/yes

@lfu lfu deleted the service_vars_1678136 branch November 4, 2019 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrafanie jrafanie jrafanie left review comments

@bdunne bdunne bdunne left review comments

@tinaafitz tinaafitz tinaafitz approved these changes

@gmcculloug gmcculloug gmcculloug approved these changes

Assignees

@gmcculloug gmcculloug

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@lfu @coveralls @tinaafitz @miq-bot @jrafanie @bdunne @gmcculloug