Miq shortcut seeding #14915
Miq shortcut seeding #14915
Conversation
There was a problem hiding this comment.
The fix seems fine, but noticed a few things that may or may not need to be addressed. Possibly someone with more familiarity around this might be able to comment.
Also, not sure why this is data that is in the DB in the first place, but that is not my concern... 🍵 🐸
| @@ -20,8 +26,7 @@ def self.seed | |||
| end | |||
| end | |||
|
|
|||
| all.each do |rec| | |||
| next if names.include?(rec.name) | |||
| db_data.each do |name, rec| | |||
There was a problem hiding this comment.
Seems like this block is unnecessary as you are now doing the delete above? Or am I reading this wrong?
There was a problem hiding this comment.
We only delete the records from the local list.
The records left in db_data are the records that did not have a corresponding seed.
There was a problem hiding this comment.
Yeah, I see what you are doing now, thanks for the clarification. Looks good. 👍
There was a problem hiding this comment.
well, that delete was causing duplicate records in the database. Took an approach more similar to what was there before.
app/models/miq_shortcut.rb
Outdated
| if db_records.size != db_data.size | ||
| names = db_records.group_by(&:name).select { |n, v| v.size > 1 }.map(&:first) | ||
| _log.warn("Duplicate seeds for names: #{names.join(",")}") | ||
| end |
There was a problem hiding this comment.
So this warning is only going to go off when re-running the MiqShortcut.seed after the data has been entered, correct? So this might not catch things right away. Should we maybe be running this logic on the seed_data keys instead?
|
fixed to no longer keep adding extra records when the seed data is bad.
|
|
This pull request is not mergeable. Please rebase and repush. |
|
/cc @isimluk we'll get the seeds down to no time flat before you know it ;) |
|
Checked commits kbrock/manageiq@20a2816~...1ec3644 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
| s[:sequence] = index | ||
| rec = find_by(:name => s[:name]) | ||
| rec = db_data[s[:name]] |
There was a problem hiding this comment.
What about db_data.delete(s[:name]) instead?
Then we can remove next if from the last loop (making it quicker/no-op in most cases).
There was a problem hiding this comment.
Discussed on pm with @kbrock. Not a blocker for this pr.
|
@kbrock I am afraid we may need a bz at this point, if you want this to be fine/yes. |
|
meh, good call @isimluk - just saying no. |
fixMiqShortcutfixture (miq_shortcut.csv) to remove duplicate name.Now populate bothcloud / snapshotsandcloud / volumesinto the databaseMiqShortcut#seedUPDATE: #14914 took care of the duplicate shortcut fixture
initial seed population
commentsbefore-3after-4seed update
commentsbefore-update-3after-update-4