Allow version validation to ignore NiFi or Registry when checking for parameter support

[ChrisSamo632]

Closes #301

[ottobackwards]

can you change the if else structure to match the rest of the code?
if X
do
else

[ChrisSamo632]

👍

[ottobackwards]

Is there a way to add a test for this?

[ChrisSamo632]

Is there a way to add a test for this?

Not that I could see easily because the methods it's calling actually throw an error if the versions don't match and I couldn't see a way of changing the service version as part of a test either (and writing integration tests that need people to change the versions of nifi/registry they're running against part way through just seemed silly)

I'm not convinced the existing method's approach of throwing an Error is correct either (the warning message on this validate method would presumably never be reached because the error would be raised)... but changing that existing behaviour didn't seem sensible as part of this PR, that should be considered as a separate Issue if someone with more knowledge of the existing logic agreed (in my opinion)

[Chaffelson]

Thanks for the fix Chris, and the review Otto. I'll try and get this tested and merged by Monday

…

On Sat, May 7, 2022, 6:43 AM Chris Sampson ***@***.***> wrote: Reopened #304 <#304>. — Reply to this email directly, view it on GitHub <#304 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZAZOBFJ5NTKQDGTQTGQKTVIX7HRANCNFSM5VC7L3TQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Chaffelson]

I think modifying utils.check_version to not raise an error, but simply assume registry version 0.2.0 if it cannot retrieve the swagger.json is the safer option. Throwing an error is overkill and my implementation was heavy handed - the user can have the option to proceed or halt if they cannot do the check using your true/false option.

I think check_version like this should work, if you care to examine it against your requirements:

def check_version(base, comparator=None, service='nifi', default_version='0.2.0'):
    """
    Compares version base against either version comparator, or the version
    of the currently connected service instance.

    Since NiFi is java, it may return a version with -SNAPSHOT as part of it.
    As such, that will be stripped from either the comparator version or
    the version returned from NiFi

    Args:
        base (str): The base version for the comparison test
        comparator (optional[str]): The version to compare against
        default_version (optional[str]): The version to assume the service is
            if the check cannot be completed
        service (str): The service to test the version against, currently
            only supports NiFi

    Returns (int): -1/0/1 if base is lower/equal/newer than comparator

    """
    assert isinstance(base, six.string_types)
    assert comparator is None or isinstance(comparator, six.string_types)
    assert service in ['nifi', 'registry']
    ver_a = version.parse(base)
    if comparator:
        # if b is set, we compare the passed versions
        comparator = strip_snapshot(comparator)
        ver_b = version.parse(comparator)
    elif service == 'registry':
        try:
            config = nipyapi.config.registry_config
            if config.api_client is None:
                config.api_client = nipyapi.registry.ApiClient()
            reg_swagger_def = config.api_client.call_api(
                resource_path='/swagger/swagger.json',
                method='GET', _preload_content=False,
                auth_settings=['tokenAuth', 'Authorization']
            )
            reg_json = load(reg_swagger_def[0].data)
            ver_b = version.parse(reg_json['info']['version'])
        except nipyapi.registry.rest.ApiException:
            log.warning("Unable to retrieve swagger.json from registry "
                        "to check version, assuming version %s" % default_version)
            ver_b = version.parse(default_version)
    else:
        # Working with NiFi
        ver_b = version.parse(
            strip_snapshot(
                # This call currently only supports NiFi
                nipyapi.system.get_nifi_version_info().ni_fi_version
            )
        )
    if ver_b > ver_a:
        return -1
    if ver_b < ver_a:
        return 1
    return 0

[ChrisSamo632]

@Chaffelson I think that looks OK

I think my 2nd paragraph above about Errors was more aimed at the enforce_version method, but upon re-reading that, I think I was mistaken in my comment - the bool_response is set to True as part of the validate... method meaning the Error isn't raised and the (validate) warning is reached

Your change to check_version would indeed avoid the nested extra Error being raised, where instead the new response with assumed Registry default version would seem more sensible

Do we want that change as part of this MR or a separate one?

[Chaffelson]

I've got the function here, and I'm going to make some minor fixes as part of the next release, so I'll merge this and apply the updated function myself.