Fix: Expand regular expression for version #68

SamiSousa · 2019-03-13T16:52:18Z

Combined regex patterns
Added optional version suffix

This adds support for versions that use a string after the semver.
Replaces #65

kevinrizza · 2019-03-13T18:09:37Z

@SamiSousa Could you add a unit test that ensures success for the version suffix condition?

MartinBasti

I miss unittest to prove your implementation is right

MartinBasti · 2019-03-13T18:18:37Z

operatorcourier/validate.py

-            pattern1 = re.compile(r'v(\d+\.)(\d+\.)(\d)')
-            pattern2 = re.compile(r'(\d+\.)(\d+\.)(\d)')
-            return pattern1.match(field) or pattern2.match(field)
+            pattern = re.compile(r'(v)?(\d+\.)(\d+\.)(\d)(-[a-z0-9\.])*')


Do you need all ()? You are not returning any substring from field

r'v?\d+[.]\d+[.]\d(-([a-z0-9][.]?)+)*'

I miss unittests.
Actually this is not working as you expect. (-[a-z0-9\.])* matches -a-.-2 instead -a.2. How about uppercase letters and underscores, are they allowed?
Also If I remember correctly quay doesn't allow leading zeroes

For the AppRegistry, quay allows any valid semver. Things like 0.0.1 and 1.21.5-beta1 are valid.

I agree that we can remove extraneous groups. I'd actually prefer using a proven semver library rather than reinventing the regex.

Scratch what I said, this version in the CSV has no relation to quay's AppRegistry, my mistake.
In any case, there should be a doc that outlines what format this version field can take. From experience I have seen versions that use a semver followed by a tag. I agree that -a-.-2 doesn't look right, so I'll update it to avoid something like that

It'd be better to use an external semver validator such as py package semver to avoid having to test the version regex we've implemented, especially as it becomes increasingly more complex.

SamiSousa · 2019-03-20T19:06:32Z

operatorcourier/validate.py

-            pattern2 = re.compile(r'(\d+\.)(\d+\.)(\d)')
-            return pattern1.match(field) or pattern2.match(field)
+            version = re.compile(r'v?\d+\.\d+\.\d+'
+                r'(-[A-Za-z0-9]+([-\.]?[A-Za-z0-9]+)*)?'


@alecmerdler Is there a standard for what a valid CSV version field is?

Just standard SemVer (major.minor.patch).

I've seen versions such as 0.0.1 and 1.22.0-beta1, so I've set this regex to match those as well.

But major.minor.patch isn't the full standard for Semantic Versioning:

https://semver.org/#spec-item-9

Is prerelease tagging supported? If so, we should check for it.

@kevinrizza Based on what I've seen, prerelease tagging is supported, and should be checked fo. I suggest that we use an external semver validator such as py package semver

SamiSousa · 2019-03-20T21:10:25Z

@kevinrizza Added the semver package for semver parsing, updated the Pipfile appropriately. Updated the example version to match a valid version value

operatorcourier/validate.py

kevinrizza · 2019-03-20T21:15:57Z

tests/test_validate.py

@@ -46,7 +46,7 @@ def test_ui_valid_bundle_io(bundle, expected_validation_results_dict):
            {'errors': [
                "csv.spec.links must be a list of name & url pairs.",
                "spec.version invalid is not a valid version "
-                "(example of a valid version is: v1.0.12)",
+                "(example of a valid version is: 1.0.12)",


Can we make sure to have both a positive and a negative test?

I'll use v<anything> as the negative test for this field

Actually, since this is already the negative, do you mean we need a positive test case?

For the positive test case, isn't the valid.bundle.yaml sufficient? Or should I test additional valid semvers? I don't think I need to test several semvers this since we're using an external package, so the valid bundle and the invalid bundle should be enough.

SamiSousa · 2019-03-27T19:02:31Z

/cc @galletti94

- Use semver.parse on CSV version field This adds support for versions that use a string after the semver. Eg: '0.10.1', '5.4.3-alpha1' and '0.0.3-beta5+build.3' are valid

SamiSousa · 2019-03-27T19:17:13Z

@MartinBasti @kevinrizza I've rebased my PR, please let me know if additional changes are needed

kevinrizza · 2019-03-29T12:17:11Z

/lgtm

openshift-ci-robot requested review from awgreene and MartinBasti March 13, 2019 16:52

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 13, 2019

SamiSousa mentioned this pull request Mar 13, 2019

Fix: UI validation functions #65

Closed

MartinBasti suggested changes Mar 13, 2019

View reviewed changes

SamiSousa force-pushed the version-regex branch from 041419b to cd77149 Compare March 15, 2019 19:38

SamiSousa commented Mar 20, 2019

View reviewed changes

SamiSousa force-pushed the version-regex branch from cd77149 to ca5026e Compare March 20, 2019 21:08

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 20, 2019

kevinrizza requested changes Mar 20, 2019

View reviewed changes

MartinBasti mentioned this pull request Mar 26, 2019

Add flatten command #85

Merged

SamiSousa force-pushed the version-regex branch from ca5026e to 6806361 Compare March 27, 2019 18:27

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2019

openshift-ci-robot requested a review from gallettilance March 27, 2019 19:02

SamiSousa force-pushed the version-regex branch from 6806361 to 7cd235c Compare March 27, 2019 19:09

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2019

Fix: Import semver package for version validation

e793ea7

- Use semver.parse on CSV version field This adds support for versions that use a string after the semver. Eg: '0.10.1', '5.4.3-alpha1' and '0.0.3-beta5+build.3' are valid

SamiSousa force-pushed the version-regex branch from 7cd235c to e793ea7 Compare March 27, 2019 19:12

gallettilance approved these changes Mar 27, 2019

View reviewed changes

MartinBasti approved these changes Mar 28, 2019

View reviewed changes

openshift-ci-robot assigned kevinrizza Mar 29, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2019

kevinrizza approved these changes Mar 29, 2019

View reviewed changes

kevinrizza merged commit 830872b into operator-framework:master Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Expand regular expression for version #68

Fix: Expand regular expression for version #68

SamiSousa commented Mar 13, 2019

kevinrizza commented Mar 13, 2019

MartinBasti left a comment

MartinBasti Mar 13, 2019

SamiSousa Mar 14, 2019

SamiSousa Mar 14, 2019

SamiSousa Mar 15, 2019

SamiSousa Mar 20, 2019

SamiSousa Mar 20, 2019

alecmerdler Mar 20, 2019

SamiSousa Mar 20, 2019

kevinrizza Mar 20, 2019

SamiSousa Mar 20, 2019

kevinrizza Mar 20, 2019

SamiSousa commented Mar 20, 2019

kevinrizza Mar 20, 2019

SamiSousa Mar 22, 2019

SamiSousa Mar 22, 2019

SamiSousa Mar 27, 2019 •

edited

Loading

SamiSousa commented Mar 27, 2019

SamiSousa commented Mar 27, 2019

kevinrizza commented Mar 29, 2019

Fix: Expand regular expression for version #68

Fix: Expand regular expression for version #68

Conversation

SamiSousa commented Mar 13, 2019

kevinrizza commented Mar 13, 2019

MartinBasti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamiSousa commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamiSousa Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

SamiSousa commented Mar 27, 2019

SamiSousa commented Mar 27, 2019

kevinrizza commented Mar 29, 2019

SamiSousa Mar 27, 2019 •

edited

Loading