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

Transfer IsoDatastore records to Storage and drop table #672

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

nasark
Copy link
Member

@nasark nasark commented Nov 17, 2022

  • schema changes to facilitate transfer of :iso_datastore records to :storage table
  • iso datastores will be modelled as Storage objects with STI :type => ManageIQ::Providers::Ovirt::InfraManager::IsoDatastore

Depends on:

@miq-bot add_labels enhancement, technical debt
@miq-bot assign @agrare

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

  • failure is legit but trivial - just need to remove the table from that list.
  • cops hash/parens is legit byt trivial
  • ignore timestamp cop

@agrare
Copy link
Member

agrare commented Nov 18, 2022

@nasark reminded me about the IsoImage model, so we'll need to:

  1. Add a references to iso_images -> storages
  2. Update all iso_images on the old iso_datastore to set the new storage_id column
  3. Delete the iso_datastore_id from the iso_images before dropping the iso_datastores table

Comment on lines 8 to 9
t.bigint :ems_id
t.datetime :last_refresh_on
Copy link
Member

Choose a reason for hiding this comment

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

Was this table really just 2 columns (and id)? Seems almost pointless 😮

Copy link
Member

Choose a reason for hiding this comment

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

Yes there could only be one per EMS so there wasn't even an ems_ref or name "needed" even though it would be helpful

@nasark nasark force-pushed the iso_datastore_schema_changes branch 2 times, most recently from a76f1fc to 1bba9b1 Compare November 23, 2022 21:04
@nasark
Copy link
Member Author

nasark commented Nov 23, 2022

Thanks for the feedback all, I have made the changes and would appreciate another look whenever possible.

Comment on lines 14 to 32
emss.map do |ems|
datastore_stub.create!(:ext_management_system => ems)
end
Copy link
Member

Choose a reason for hiding this comment

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

No need for .map if you aren't using the return

Suggested change
emss.map do |ems|
datastore_stub.create!(:ext_management_system => ems)
end
emss.each { |ems| datastore_stub.create!(:ext_management_system => ems) }

@agrare
Copy link
Member

agrare commented Nov 28, 2022

@nasark can we add specs covering the IsoImage changes as well?

@nasark nasark force-pushed the iso_datastore_schema_changes branch from c9a0762 to 071578d Compare November 29, 2022 21:21
@nasark nasark force-pushed the iso_datastore_schema_changes branch from 071578d to e3385a5 Compare November 29, 2022 21:46
@miq-bot
Copy link
Member

miq-bot commented Nov 29, 2022

Checked commit nasark@e3385a5 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 1 offense detected

db/migrate/20221107195559_drop_iso_datastore.rb

@agrare agrare merged commit 6c8f536 into ManageIQ:master Dec 14, 2022
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.

5 participants