-
Notifications
You must be signed in to change notification settings - Fork 107
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
update aws-sdk gem to v3 #488
Conversation
manageiq-providers-amazon.gemspec
Outdated
@@ -13,7 +13,7 @@ Gem::Specification.new do |s| | |||
|
|||
s.files = Dir["{app,config,lib}/**/*"] | |||
|
|||
s.add_dependency("aws-sdk", ["~>2.9.7"]) | |||
s.add_dependency("aws-sdk", ["~> 3"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment regarding the gem version, and also remove all of the parens and square brackets (go for consistency with the lines below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
404c1be
to
ab899ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexanderZagaynov When the other gem dependency PRs are merged and travis can start running again, I am guessing that #434 is going to start failing, and that PR and #432 are going to have to be removed.
Thinking you probably can wait on that until I am correct in that assumption, but know that it will probably be something you need end up doing in the end.
@NickLaMuro If you mean I should revert / adopt those commits - I'll do. |
@NickLaMuro that was fixed in aws/aws-sdk-ruby#1760, thanks to @Fryguy :) So, I'm simply remove that patch entirely. |
ab899ac
to
61eb6d8
Compare
519ad9a
to
e5e5eb2
Compare
app/models/manageiq/providers/amazon/network_manager/refresh_parser.rb
Outdated
Show resolved
Hide resolved
Aside from my comment above, this LGTM. @NickLaMuro not sure why you have a request change on this, but what do you think now? |
@Fryguy was mostly using that as a reminder that the PRs that I merged to patch Which they have. But also, that I was suggesting that we wait to confirm the tests failed when the other PRs have been fixed, addressed and merged, since those should be merge-able without requiring other PRs to work. Reason being is that test is in there to confirm we don't have a regression since the fix ( #432 ) really didn't provide a spec of it's own to validate. Since it seems that Anyway, I will remove my ❌ since it doesn't seem like there is an easy solution for this. |
@NickLaMuro other repos were updated as well. |
@AlexanderZagaynov I am referring to why Travis is currently failing:
To get this to pass prior to merging this (in CI), and do what I suggested in the previous comment would probably have some undesirable affects for the rest of the team in the mean time. Again, I removed my "Request changes" review. |
@AlexanderZagaynov What's the word? |
@djberg96 |
0ad8451
to
e5904f0
Compare
@AlexanderZagaynov I think you'll need to update the gemspec, too. |
@AlexanderZagaynov Oh, I didn't see it. I guess it's gems-pending that's the culprit. |
Checked commits AlexanderZagaynov/manageiq-providers-amazon@32d337d~...8be7e04 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
All tests are green on my machine with patched versions of all gems mentioned here. |
@Fryguy @NickLaMuro @agrare please review |
Will merge when the cross-repo test is green https://travis-ci.org/agrare/manageiq-cross_repo/builds/600907361 |
This is a part of global update
aws-sdk
gem tov3
Together with:
It should fix #465