replica install: relax domain level check for promotion

[frasertweedale]

promote_check currently requires DL == 1. Relax the check to
require DL >= 1, so that things will work for future DL increases.

Part of: https://fedorahosted.org/freeipa/ticket/5011

[frasertweedale]

Build failure is unrelated to patch.

[HonzaCholasta]

Excuse me, but what is the point of checking for an exact domain level? Shouldn't check_domain_level() rather always check for a minimum domain level?

[MartinBasti]

expected is for domain level 0, because there are different expectations about replica file, it must exactly match domain level 0, you cannot have higher DL.

[HonzaCholasta]

I see. The point is, check_domain_level() is supposed to check whether replica promotion is possible or not in the current domain level, so it's weird it has an expected domain level argument and even weirder to introduce additional minimum domain level argument, when all it should have is a single boolean argument saying wheter you want to promote or not:

def check_domain_level(api, want_promote):
    ...

    promote = current >= constants.DOMAIN_LEVEL_1
    if promote != want_promote:
        raise RuntimeError(message)

    ...

[MartinBasti]

IMO the whole check_domain_level is somehow broken, AFAIK the main purpose of it is to print correct error message related to replica file option, depending on current and expected domain level.

@stlaz may know more details

[frasertweedale]

So, what do we want the behaviour of check_domain_level to be? I just want to make a small change so that replica install does not break if DL > 1.

[frasertweedale]

@HonzaCholasta @MartinBasti PR updated. I extracted the specific (== 0) and (>= 1) checks to the relevant call sites. Also separated DL retrieval and "DL in range for IPA version" check into separate functions.

[stlaz]

The purpose of check_domain_level() was to have a unified means of checking whether the domain level in the rest of the domain corresponds to the installation media which is presented by the user.

Looking back at it I think I chose poor naming and documentation of the check so it's rather confusing.
If you find a way to make a unified check in both domain levels (=> a single function call for both DLs that will raise exception when wrong installation media is presented), that'd be nice. Otherwise feel free to split it back to what it was previously.

[frasertweedale]

@stlaz there are three considerations when "checking the DL":

1. Retrieving the current DL.
2. Checking that current DL is supported by server version.
3. Checking that attempted method of installation is supported on currently DL.

Whether it makes sense to have a unified function for (3), I am not sure. I think the approach as implemented in this PR - that each replica installation method checks the DL and if necessary raises an appropriate error message - is satisfactory. Certainly it makes more sense to me to have these checks separate from the check for (2).

[stlaz]

@frasertweedale Alright. I am definitely not against having it separated since we came to the realization that replica install checks can not be merged in a satisfactory way anyway.

[frasertweedale]

Any other changes requested? What's preventing ack on this?

[MartinBasti]

ACK and I found a new bug: https://fedorahosted.org/freeipa/ticket/6654

[MartinBasti]

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/f51869bf5214e2d2322f85bf72b7ae86b6893974