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

Handle possibility of no orchestration stacks #84

Merged
merged 1 commit into from
Jun 27, 2017
Merged

Handle possibility of no orchestration stacks #84

merged 1 commit into from
Jun 27, 2017

Conversation

djberg96
Copy link
Collaborator

Fixes a potential bug in the get_stack_templates method.

Addresses #83

@djberg96
Copy link
Collaborator Author

@miq-bot add_label fine/yes

@djberg96
Copy link
Collaborator Author

@miq-bot add_label euwe/yes

@bzwei
Copy link
Contributor

bzwei commented Jun 26, 2017

LTGM. Thanks.

@bronaghs
Copy link

@djberg96 - any chance of a spec?

@djberg96
Copy link
Collaborator Author

djberg96 commented Jun 27, 2017

@bronaghs @bzwei I will need some help with that. :)

@blomquisg
Copy link
Member

@djberg96 the easiest way I can see testing this is to refactor the way that deployments are collected by adding a get_deployments method that simply calls get_data_for_this_region(@tds, 'list').

Then, you can easily stub out that method to return an empty array, or nil, or whatever you need it to return to reproduce this situation. You could probably even stub that out in the VCR-based specs, but that might result in the dreaded "Unused HTTP Calls" error.

Without that wrapper method, it looks like lots and lots of digging holes to stub out weird things.

#justathought

Add spec for possibly empty template deployment list.
@djberg96
Copy link
Collaborator Author

@blomquisg I was able to figure it out. :)

That said, I did split out a get_deployments method. While not strictly necessary for this PR, I included it anyway to make future testing potentially easier.

@bronaghs @bzwei Please review.

@miq-bot
Copy link
Member

miq-bot commented Jun 27, 2017

Checked commit https://github.com/djberg96/manageiq-providers-azure/commit/2d4a7fccf144b3734e355e9a1da9153aee65c7c3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 2 offenses detected

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

@blomquisg blomquisg merged commit cb8acf4 into ManageIQ:master Jun 27, 2017
@blomquisg blomquisg added the bug label Jun 27, 2017
@blomquisg blomquisg added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 27, 2017
@simaishi
Copy link
Contributor

@djberg96 Is there a BZ for this? Can you please create if it doesn't exist?

@djberg96
Copy link
Collaborator Author

djberg96 commented Jul 3, 2017

@simaishi
Copy link
Contributor

simaishi commented Jul 7, 2017

@djberg96 app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb is giving a conflict backporting to Euwe. Please resolve conflict and create an Euwe-specific PR (referencing this one) or suggest other PRs to backport.

++<<<<<<< HEAD
 +        old_status_message = get_resource_status_message(old_resource)
 +
 +        old_resource.properties['status_message'] = if old_status_message
 +                                                      "#{old_status_message}\n#{new_status_message}"
 +                                                    else
 +                                                      new_status_message
 +                                                    end
 +      end
 +
 +      def get_stack_template(stack, content)
 +        process_collection([stack], :orchestration_templates) { |the_stack| parse_stack_template(the_stack, content) }
++=======
+         # link stacks to templates, convert raw_template to template
+         Hash(@data_index[:orchestration_stacks]).each do |_stack_uid, stack|
+           raw_template = stack[:orchestration_template]
+           stack[:orchestration_template] = @data_index.fetch_path(:orchestration_templates, raw_template[:uid])
+         end
++>>>>>>> cb8acf4... Merge pull request #84 from djberg96/orchestration_stack_fix
        end

@djberg96
Copy link
Collaborator Author

djberg96 commented Jul 7, 2017

@simaishi I don't know that it needs backporting to Euwe. @bzwei The logic changed between Euwe and Fine, didn't it?

@blomquisg
Copy link
Member

@djberg96 you added the euwe/yes label :)

@djberg96
Copy link
Collaborator Author

djberg96 commented Jul 7, 2017

@blomquisg dang, i suck.

@djberg96
Copy link
Collaborator Author

djberg96 commented Jul 7, 2017

@miq-bot add_label euwe/no

@djberg96
Copy link
Collaborator Author

djberg96 commented Jul 7, 2017

@simaishi I will submit a separate PR for Euwe. Sorry about that.

@miq-bot miq-bot added the euwe/no label Jul 7, 2017
@djberg96
Copy link
Collaborator Author

djberg96 commented Jul 7, 2017

@miq-bot remove_label euwe/yes

@miq-bot miq-bot removed the euwe/yes label Jul 7, 2017
@simaishi
Copy link
Contributor

simaishi commented Jul 7, 2017

Fine backport details:

$ git log -1
commit 0bea1a1b12325dfb3cbc9630158369cb72214fc8
Author: Greg Blomquist <[email protected]>
Date:   Tue Jun 27 16:27:31 2017 -0400

    Merge pull request #84 from djberg96/orchestration_stack_fix
    
    Handle possibility of no orchestration stacks
    (cherry picked from commit cb8acf4422799d9861f4e43a9c07d7038fbe7405)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1468703

@simaishi
Copy link
Contributor

simaishi commented Jul 7, 2017

Backported to Euwe via ManageIQ/manageiq#15527

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.

6 participants