version test - improve message when value is empty #74754
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When an empty value is provided, no `version` attribute will exist on the `LooseVersion` or `StrictVersion` object. We catch and handle this, but it's not immediatebly clear that an AttributeError means an empty value was provided. Handle this case specifically and add a more helpful error message. Add unit and integration tests.
ansibot
added
affects_2.12
bug
sivel
reviewed
May 18, 2021
| @@ -168,6 +168,9 @@ def version_compare(value, version, operator='eq', strict=None, version_type=Non | |||
| if strict is not None and version_type is not None: | |||
| raise errors.AnsibleFilterError("Cannot specify both 'strict' and 'version_type'") | |||
|
|
|||
| if not value: | |||
There was a problem hiding this comment.
I think we need to check both version and value. Otherwise just catch the AttributeError in the below try/except and give this error there.
There was a problem hiding this comment.
Both should be checked.
Do we want one error message, or a separate error message for each combination (value, version, both)?
There was a problem hiding this comment.
Catching the AttributeError was the first approach I took. Then I switched to failing early by checking up front. I didn't think about checking both value and version, though.
I'll just test both here and adjust the error message.
There was a problem hiding this comment.
I went with separate error messages for both. The language was too ambiguous trying to cram it into one message.
There was a problem hiding this comment.
I'm open to feedback on the language. It's tough trying to communicate the input version value versus the version comparison value. They're both version comparison values. 🤯
mattclay
requested changes
May 18, 2021
| @@ -168,6 +168,9 @@ def version_compare(value, version, operator='eq', strict=None, version_type=Non | |||
| if strict is not None and version_type is not None: | |||
| raise errors.AnsibleFilterError("Cannot specify both 'strict' and 'version_type'") | |||
|
|
|||
| if not value: | |||
There was a problem hiding this comment.
Both should be checked.
Do we want one error message, or a separate error message for each combination (value, version, both)?
ansibot
added
needs_revision
Add different error messages for each. Not handling the scenario if both are empty for now. Update integration tests.
ansibot
added
core_review
mattclay
approved these changes
May 19, 2021
mattclay
removed
the
needs_triage
ansibot
added
core_review
samdoran
added a commit
to samdoran/ansible
that referenced
this pull request
May 21, 2021
…ible#74754) When an empty value is provided, no `version` attribute will exist on the `LooseVersion` or `StrictVersion` object. We catch and handle this, but it's not immediatebly clear that an AttributeError means an empty value was provided. Specifically handle the case where value or version are empty and add more helpful error messages. Add integration tests. (cherry picked from commit 71e33d2) Co-authored-by: Sam Doran <sdoran@redhat.com>
relrod
pushed a commit
that referenced
this pull request
Jun 11, 2021
) (#74789) When an empty value is provided, no `version` attribute will exist on the `LooseVersion` or `StrictVersion` object. We catch and handle this, but it's not immediatebly clear that an AttributeError means an empty value was provided. Specifically handle the case where value or version are empty and add more helpful error messages. Add integration tests. (cherry picked from commit 71e33d2) Co-authored-by: Sam Doran <sdoran@redhat.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
When an empty value is provided, no
versionattribute will exist on theLooseVersionorStrictVersionobject. We catch and handle this, but it's not immediatebly clear to the non-Python programmer that anAttributeErrormeans an empty value was provided to the test.Handle this case specifically and add a more helpful error message.
Add unit and integration tests.
ISSUE TYPE
COMPONENT NAME
lib/ansible/plugins/test/core.py