[Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users

[Simo Sorce]

On Fri, 2016-07-29 at 15:19 +0200, Martin Basti wrote:
> 
> On 29.07.2016 15:12, Simo Sorce wrote:
> > On Fri, 2016-07-29 at 15:10 +0200, Martin Basti wrote:
> >> On 29.07.2016 14:42, Florence Blanc-Renaud wrote:
> >>> On 07/28/2016 10:56 AM, Martin Babinsky wrote:
> >>>> Fixes https://fedorahosted.org/freeipa/ticket/6101
> >>>>
> >>>> I have also noticed that the principal aliases are not preserved during
> >>>> migration from FreeIPA 4.4.
> >>>>
> >>>> That, however, requires more powerful runes to transform the realm of
> >>>> all values and warrants a separate ticket if we even want to support
> >>>> migration of user aliases.
> >>>>
> >>>>
> >>>>
> >>> Hi Martin,
> >>>
> >>> thanks for your patch. From a technical standpoint, it looks good to
> >>> me as I tested the following scenarios:
> >>>
> >>> 1/ without --user-ignore-attribute
> >>> - call ipa migrate-ds without specifying any attributes to ignore
> >>> The user entries are migrated, and contain a migrated krbprincipalname
> >>> and krbcanonicalname.
> >>> At this point kinit fails but this is expected as the krb attributes
> >>> were not re-generated. Login to the web https://hostname/ipa/ui also
> >>> fails as expected.
> >>> - login to https://hostname/ipa/migration with the user credentials
> >>> - perform kinit => OK
> >>> - login to https://hostname/ipa/ui => OK
> >>>
> >>> 2/ with --user-ignore-attribute={krbPrincipalName,krbextradata,...} as
> >>> explained in the Migration page [1]
> >>> At this point kinit fails as expected, as well as login to the web
> >>> ipa/ui.
> >>> - login to https://hostname/ipa/migration with the user credentials
> >>> - perform kinit => OK
> >>> - login to https://hostname/ipa/ui => OK
> >>>
> >>>
> >>> But the patch produces new pep8 complaints:
> >>> ./ipaserver/plugins/migration.py:39:1: E402 module level import not at
> >>> top of file
> >> This is caused by old code, it should not prevent this patch to be
> >> acked. Imports are heavily mixed in code already, it is not possible to
> >> keep importing right without fixing old ones.
> >> Martin^2
> > Can we make a patch to fix all import order issues across the code base
> > so we can maintain them properly going forward ?
> >
> > Simo.
> >
> 
> Is it worth it?
> 
> a) it will makes git blame harder, you will have to go always deeper to 
> history with any import

I considered this, but I rarely found that imports were such a big
issue, usually it's code in the file that is, so IMO the trade-off is
worth it.

> b) it makes backporting of patches harder. We fixed unused imports in 
> 4.4, and it was PITA to backport patches, always conflicts, several 
> bugs. Changing all import to correct order will be even worse.

Sure backports will be somewhat harder, but I wouldn't say it is a
nightmare, it is not code logic that changes, it is just individual
import lines.

> However plus is, when we once fix it, we can enable pylint check to keep 
> it right forever.

Exactly, this is the appealing point, we get it right once and then the
tool keeps us honest going forward.

> We can open refactoring ticket and we'll see.

Please do.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York