Closed ChrisSamo632 closed 2 years ago
Is there a way to add a test for this?
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)
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 https://github.com/Chaffelson/nipyapi/pull/304.
— Reply to this email directly, view it on GitHub https://github.com/Chaffelson/nipyapi/pull/304#event-6567074649, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZAZOBFJ5NTKQDGTQTGQKTVIX7HRANCNFSM5VC7L3TQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
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
@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?
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.
Closes #301