-
Notifications
You must be signed in to change notification settings - Fork 900
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
Remove cfme repos from MiqDatabase model #11304
Conversation
56d9a8f
to
c3c8d87
Compare
db.update_attributes( | ||
credentials.slice( | ||
:registration_type, | ||
:registration_server, | ||
:registration_http_proxy_server, | ||
:update_repo_name | ||
:registration_http_proxy_server |
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.
Not sure I understand this change. update_attributes will call the assignment, so there should be no change needed here.
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.
This is done. Not sure how to indicate just one part of a review is completed as the "outdated diff" stuff doesn't seem to apply to parts of a review.
return unless repos | ||
hash = {:product => {:update_repo_names => repos.split}} | ||
Vmdb::Settings.save!(MiqRegion.my_region, hash) | ||
Settings.reload! |
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.
IF the save! is not doing this we need an alternate method that propogates the changes to other servers.
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.
It doesn't look like it is right now. Should I just add a reload!
at the end of the Vmdb::Settings.save!
method?
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.
Can we maybe make this change in a follow up so that we can get the migrations in or do you think it is necessary for this PR?
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.
Yeah, as discussed, follow this up, For now, this will work for a single appliance.
This will preserve the exising repo data that some customers may have modified manually before deleting the column
This will be productized and stored in Settings from now on
For this we needed to add a the region resource to properly save to update repo settings and add some strategic settings and object reloads.
3dbbcec
to
6732bc9
Compare
Checked commits carbonin/manageiq@476c561~...6732bc9 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/models/miq_database.rb
|
This PR removes the default value for the cfme yum repos out of the MiqDatabase model. This will remove the need for PRs like #11224 and #9526 every time we cut a cfme release.
This also allows us to drop the
update_repo_name
column in themiq_databases
table and move the information to the config settings which are more easily productized.@miq-bot add_label enhancement, core, darga/no
@Fryguy @gtanzillo please review