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

Enable S3 DB Backups #227

Merged
merged 2 commits into from
Aug 1, 2018
Merged

Enable S3 DB Backups #227

merged 2 commits into from
Aug 1, 2018

Conversation

jerryk55
Copy link
Member

@jerryk55 jerryk55 commented Jun 26, 2018

Add the AWS region to the file depot so S3 depots contain a region value.

For RFE referenced in https://bugzilla.redhat.com/show_bug.cgi?id=1513520

@roliveri please review. Also requires ManageIQ/manageiq-gems-pending#355
as well as a forthcoming ManageIQ core PR and a TBD manageiq-ui-classic PR.

@Fryguy please review and merge. Thanks.

Add the AWS region to the file depot so S3 depots contain a region value.
@blomquisg
Copy link
Member

@miq-bot assign @Fryguy

@@ -0,0 +1,5 @@
class AddRegionToFileDepot < ActiveRecord::Migration[5.0]
def change
add_column :file_depots, :region, :string
Copy link
Member

@jrafanie jrafanie Jul 27, 2018

Choose a reason for hiding this comment

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

A region column feels specific to amazon and not to depots. It's also a string... like the mistake we made with using zone names in the queue instead of id's referencing rows in the zone table. If we intend to use depots in other clouds, it might make sense to consider using a polymorphic relationship to that provider's storage location table.

This might be out of scope of this PR but I see the zone mistake we made with the queue when I look at this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is really just for the S3 File Depot, but there is not a separate S3 File Depot table. There will also be a depot for Swift. Can we look at your suggestion at a later point so that we can get this merged for the upcoming Sprint? There are 4 other PRs that are waiting like dominoes on this one to fall... (poor choice of words, I know)

Copy link
Member

Choose a reason for hiding this comment

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

Sure, unless we know how it will look for other systems, it's probably better to wait until we know how it will look. That is, unless the migration would be painful.

Copy link
Member Author

Choose a reason for hiding this comment

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

No we don't know at this point. Thanks for your help.

@Fryguy
Copy link
Member

Fryguy commented Jul 27, 2018

Region can't be a column because it's reserved for the region number virtual column

@jerryk55
Copy link
Member Author

@Fryguy are you talking about the name region? Can it be called something like "object_region" or "depot_region"?

@Fryguy
Copy link
Member

Fryguy commented Jul 27, 2018

I'm fine if this is AWS specific and just named aws_region it something like that

@Fryguy
Copy link
Member

Fryguy commented Jul 27, 2018

Yes the name region would conflict with our general region concept (every model has region_number, in_region, etc methods). For ext_management_systems we chose provider_region, but I'm not sure that would make sense here.

@Fryguy
Copy link
Member

Fryguy commented Jul 27, 2018

Per review comments, the name "region" is not allowed.
@jerryk55
Copy link
Member Author

@Fryguy, renamed the column "aws_region" and re-pushed. Let me know if this looks good. Thanks!

@miq-bot
Copy link
Member

miq-bot commented Jul 27, 2018

Checked commits jerryk55/manageiq-schema@5971812~...2b8ac3b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@jerryk55
Copy link
Member Author

@Fryguy can this be merged now? Thanks!

@Fryguy Fryguy merged commit b40120c into ManageIQ:master Aug 1, 2018
@Fryguy Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 1, 2018
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