search

ManageIQ / azure-armrest

Ruby interface for Azure using the new REST API
Apache License 2.0
15 stars 36 forks source link

Internal StorageAccount model fixes, other fixes #401

Closed djberg96 closed 3 years ago

djberg96 commented 3 years ago

Running the rubocop linter over the code base revealed a bug in the StorageAccount model where the transform_keys method wasn't actually doing anything. I don't think this showed up because typically the defaults are used.

Additionally, a duplicate exception was corrected, and options are now actually passed to the vm_operate method, which could potentially affect the capture method.

Lastly, a redundant require statement and some useless assignments were removed.

djberg96 commented 3 years ago

@bzwei Please review.

djberg96 commented 3 years ago

Seems I had to update the activesupport dependency, otherwise 1.month breaks with newer versions.

NickLaMuro commented 3 years ago

@djberg96 We are going to need a Rails 6.0 compatible version of this then because I have been working on that the Rails 6 upgrade for the past couple of months, and releasing something like this will probably break the build:

https://github.com/ManageIQ/manageiq/issues/19977

We are getting very close to releasing that as well.

djberg96 commented 3 years ago

@NickLaMuro Ok, updated. Near as I can tell that works with Rails 5 or 6.

miq-bot commented 3 years ago

Checked commits https://github.com/djberg96/azure-armrest/compare/e6d477745eae610154a6c059314015d1f61b3fb5~...b8e05ed0074317f49d7bdf519b8720863bc8bd34 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint 7 files checked, 0 offenses detected Everything looks fine. :cookie:

NickLaMuro commented 3 years ago

@djberg96 awesome, thanks! I was going to look into doing tackling either something like you did here or an alternative rails-6 branch for this same work, but you beat me to it!

Thanks again!

NickLaMuro commented 3 years ago

That said, it might not hurt to rebase down that last to commits, and then extract that commit to a new branch and do a release of the gem that fixes the support (since this is will be failing on master).

Actually, I will save you the trouble and throw that PR together for you.

bzwei commented 3 years ago

@djberg96 can it be merged, or you'll need to do what @NickLaMuro suggested?

djberg96 commented 3 years ago

@bzwei It can be merged.