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

update aws-sdk gem to v3 #449

Merged
merged 4 commits into from
Oct 21, 2019
Merged

Conversation

AlexanderZagaynov
Copy link
Contributor

@AlexanderZagaynov AlexanderZagaynov commented Oct 17, 2019

@miq-bot
Copy link
Member

miq-bot commented Oct 17, 2019

Checked commits AlexanderZagaynov/manageiq-gems-pending@5086ae5~...f0a5d5a with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@djberg96
Copy link
Contributor

Huh, 2.5 passed, but 2.4 failed. Maybe restart Travis on this one, see what happens.

@AlexanderZagaynov
Copy link
Contributor Author

yeah, some timeout issue, however I have no control to restart it...

@djberg96
Copy link
Contributor

@AlexanderZagaynov Neither can I. You can do the old "close and reopen" trick, I think that will kick it.

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

This is the only PR I think I had where I would have any stake in the matter. I would like to run some automated tests I had around this code, so I am going to do that first before officially approving, but I think what you have here makes sense to start.

I think long term, I would like to move these changes to be something that is included with a provider plugin instead of living in this repo, but that is a longer discussion to be had, and we should do that in a separate effort after this.

Will respond again once I can test DB backups with this change.

@djberg96
Copy link
Contributor

@NickLaMuro What specifically is at stake? Anything we can help with?

@agrare agrare self-assigned this Oct 21, 2019
@agrare
Copy link
Member

agrare commented Oct 21, 2019

Provider tests are green with this applied

@agrare agrare merged commit f92379b into ManageIQ:master Oct 21, 2019
@agrare agrare added this to the Sprint 123 Ending Oct 28, 2019 milestone Oct 21, 2019
@NickLaMuro
Copy link
Member

@djberg96 I think it is fine that this is merged without me testing (sorry... computer swapping all day), but this is your question:

@NickLaMuro What specifically is at stake? Anything we can help with?

It is currently being used for here:

https://github.com/ManageIQ/manageiq-gems-pending/blob/master/lib/gems/pending/util/object_storage/miq_s3_storage.rb

Which, as you probably can imagine, has zero unit tests around the database backup code for this. Also, we are streaming the data from the backup directly to S3, which makes it complicated. That said, what was here previously was a patch of a direct port of what existed in v3 that should work just fine.

That said, I haven't had a chance to run through my test suite of this yet, since I honestly have missed placed where I have configured it, but the test suite is basically here:

https://gist.github.com/NickLaMuro/8438015363d90a2d2456be650a2b9bc6

That said, getting that repo ("test suite") up and running is no easy task, so I just need to find some time to update things and make sure this new code works with the s3 tests still. On my TODO list...

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.

Upgrade AWS SDK library to version 3
5 participants