On 09/09/2011 01:11 AM, Mo Morsi wrote:
I ran into these issues myself, and was going to post similar
feedback
before I noticed the reply. See additional comments inline
On 08/26/2011 04:55 PM, Matt Wagner wrote:
> ---
> src/db/migrate/20110329104633_rename_ssh_user.rb | 16 ++++++++++------
> .../migrate/20110513160000_rename_rhevm_target.rb | 12 ++++++++----
> ...110707082304_rename_rhevm_and_vmware_targets.rb | 6 ++++--
> .../migrate/20110811091500_rename_condorcloud.rb | 2 ++
> 4 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/src/db/migrate/20110329104633_rename_ssh_user.rb
b/src/db/migrate/20110329104633_rename_ssh_user.rb
> index e77eae3..f13ae1f 100644
> --- a/src/db/migrate/20110329104633_rename_ssh_user.rb
> +++ b/src/db/migrate/20110329104633_rename_ssh_user.rb
> @@ -1,15 +1,19 @@
> class RenameSshUser< ActiveRecord::Migration
> def self.up
> pt = ProviderType.first(:conditions => {:codename =>
'ec2'})
> - pt.ssh_user = 'root'
> - pt.home_dir = '/root'
> - pt.save!
> + if pt
> + pt.ssh_user = 'root'
> + pt.home_dir = '/root'
> + pt.save!
> + end
> end
Overall these changes work, though since the seed data has been moved
to seed.rb, the migrations affected here are somewhat unnecessary (these
migrations rely on the seed data, while the seed data relies on the
migrations). Instead the changes in these migrations can be added to the
seed data themselves and these migrations can be removed all together.
That's exactly why this is necessary. Migrations should only touch data
that's already there -- i.e. fixing existing data in a real
installation. If the db is blank when the migration is run, it shouldn't
add any rows. Matt added these conditions because I added a migration
that changed the columns in ProviderType, so any time these old
migrations are run, they'll throw an error if they touch ProviderTypes
-- that's why this fix was added -- with my patchset and without this
patch, things blow up on a fresh db.
No objections to pushing these patches, but at some point we should
cleanup / consolidate what belongs in the migrations and what belongs in
the seed data.
Actually, we did that months ago -- seed data includes all data
necessary to run for a new database. What we're seeing here is that
after we scrubbed the migrations of seed data some months back, a few
migrations have been written that were less careful _not_ to add data to
a new database.
Scott