Skip to content

Fix load_templates --clean to wipe more than one entry per table #2955

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

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

AdamWill
Copy link
Contributor

1400d8a inverted the logic here - instead of exiting the loop
on a failure we now exit it on success. So each time --clean
is run it will only wipe one entry per table. This obviously
breaks things badly (the clean phase doesn't clean things right
at all, so the subsequent load phase fails because we try to add
entries that already exist, and you wind up with a mess).

This flips the logic back, and adds a test.

Signed-off-by: Adam Williamson awilliam@redhat.com

@AdamWill
Copy link
Contributor Author

@kalikiana @okurz

@AdamWill AdamWill force-pushed the fix-templates-clean branch 3 times, most recently from 7d054ab to edbef94 Compare April 18, 2020 19:42
@AdamWill
Copy link
Contributor Author

The failure here doesn't look like anything to do with the PR.

@okurz okurz requested a review from kalikiana April 20, 2020 05:09
@okurz
Copy link
Member

okurz commented Apr 20, 2020

yes, ui test failure is unrelated, should be fixed meanwhile with a PR merged to master, retriggered test here.

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

This makes generally sense. But let's have @kalikiana have a look at it, too.

@@ -241,7 +241,7 @@ if ($options{'clean'}) {
my $id = $result->{$table}->[$i]->{id};
my $table_url_id = $url->clone->path($options{'apibase'} . '/' . decamelize($table) . "/$id");
$res = $client->delete($table_url_id)->res;
last if $res->is_success;
last unless $res->is_success;
Copy link
Member

Choose a reason for hiding this comment

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

I did not realize I'd flipped this condition. Good catch!

test_once $args, $expected, 'imported original fixtures';
is $schema->resultset('Machines')->count, 1, "only one machine is loaded";
my $machine = $schema->resultset('Machines')->first;
is $machine->name, "32bit", "correct machine is loaded";
Copy link
Member

Choose a reason for hiding this comment

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

That looks great. Thank you for coming up with the test.

1400d8a inverted the logic here - instead of exiting the loop
on a *failure* we now exit it on *success*. So each time --clean
is run it will only wipe one entry per table. This obviously
breaks things badly (the clean phase doesn't clean things right
at all, so the subsequent load phase fails because we try to add
entries that already exist, and you wind up with a mess).

This flips the logic back, and adds a test.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@okurz okurz force-pushed the fix-templates-clean branch from edbef94 to 5396d7f Compare April 20, 2020 18:39
@okurz
Copy link
Member

okurz commented Apr 20, 2020

I updated your branch. That should fix the failed test.

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #2955 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2955      +/-   ##
==========================================
+ Coverage   93.44%   93.46%   +0.01%     
==========================================
  Files         190      190              
  Lines       11959    11959              
==========================================
+ Hits        11175    11177       +2     
+ Misses        784      782       -2     
Impacted Files Coverage Δ
lib/OpenQA/Worker/CommandHandler.pm 93.75% <0.00%> (+1.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88f2d5a...5396d7f. Read the comment docs.

@kalikiana kalikiana merged commit 4df4ad1 into os-autoinst:master Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants