Fix: Expand regular expression for version by SamiSousa · Pull Request #68 · operator-framework/operator-courier

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

                "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 MartinBasti and awgreene 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

Conversation

SamiSousa commented Mar 13, 2019

Uh oh!

kevinrizza commented Mar 13, 2019

Uh oh!

MartinBasti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamiSousa commented Mar 20, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamiSousa Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamiSousa commented Mar 27, 2019

Uh oh!

SamiSousa commented Mar 27, 2019

Uh oh!

kevinrizza commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SamiSousa Mar 27, 2019 •

edited

Loading