[prev in list] [next in list] [prev in thread] [next in thread] 

List:       spacewalk-devel
Subject:    Re: [Spacewalk-devel] Review patch for satellite-sync
From:       Michael Mraka <michael.mraka () redhat ! com>
Date:       2012-02-22 14:43:09
Message-ID: 20120222144309.GB3079 () magni ! brq ! redhat ! com
[Download RAW message or body]

Parsons, Aron wrote:
% But you do get new packages with this change.  Test for yourself:
% 
% Test case:
% * rhnpush a package with a unique name into a test channel
% * export that channel
% * delete the package from the channel
% * select id from rhnpackagename where name = 'name';
% * delete from rhnpackagenevra where name_id = <id>;
% * delete from rhnpackagename where name = "<name>";
% * verify that package is not in your database
% * satellite-sync your export from above with this patch
% * the package is now in the database
% 
% Here's how it happens:
%
% - ChannelPackageSubscription::fix() from
% server/importLib/packageImport.py calls lookupPackageNames from
% server/importLib/backend.py - This iterates through all of the
% packages and calls lookup_package_name() against the 'dual' table.
% lookupPackageNames doesn't return anything, so obviously it's there to
% ensure that the package names exist at that point.
% 
% I don't think the semantics of _query_compare_packages call for a
% package name to be created; it's doing a diff to see what exists and
% what doesn't.  If anything, I think lookup_package_name is poorly
% named and used incorrectly within _query_compare_packages since it
% does more than just lookup a package name; I wouldn't expect a lookup*
% function to insert records.
% 
% Thoughts?

Hi Aron,

I reviewed and tested your patch. You're correct, the purpose of this
checks isn't to create the missing name/evr/arch ids but just to check
whether package exists or not. If they don't exist there's a different query
down the road which is supposed to create them.

I commited the patch as b32b6da20c4bb7499deb377a6f36783fabc7ed1c.  I
even went a bit further ahead and replaced also lookup_evr() and
lookup_package_arch() with table joins.

Thanks for your patch.

% /aron

Regards,

--
Michael Mráka
Satellite Engineering, Red Hat

[prev in list] [next in list] [prev in thread] [next in thread]