Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update regex to match all iterations #6839

Merged
merged 5 commits into from
Feb 2, 2023
Merged

update regex to match all iterations #6839

merged 5 commits into from
Feb 2, 2023

Conversation

emmyoop
Copy link
Member

@emmyoop emmyoop commented Feb 1, 2023

Description

The existing regex (new since nightly releases were added) had a bug and did not recognize versions without any kind of prerelease (ie. 1.5.0).

This change passes for the following

  • 1.5.0
  • 1.5.0a1
  • 1.5.0a1.dev123457+nightly

and fails for

  • 1.5
  • 1
  • 1.5.2-a1

Tested with release here

Checklist

@cla-bot cla-bot bot added the cla:yes label Feb 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@emmyoop emmyoop added the Skip Changelog Skips GHA to check for changelog file label Feb 2, 2023
@emmyoop
Copy link
Member Author

emmyoop commented Feb 2, 2023

Once this is merged it needs to be added to all the core release backports (#6792, #6791, #6790 ) and also backported to 1.4.latest since that one was already merged.

@emmyoop emmyoop marked this pull request as ready for review February 2, 2023 15:41
@emmyoop emmyoop requested a review from a team as a code owner February 2, 2023 15:41
Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only found one true issue.

It could be worth commenting each line for the next person who might have to edit this. Also adding the examples in the PR description as a comment could help as well.

Two additional notes:

  • depending on how this matching is applied you may want to have start and stop anchors (^, $) to prevent matching in the middle of bad strings. e.g. - 1.2.3.4.5.6 matches twice: once on 1.2.3 and again on 4.5.6
  • There are also two unnamed groups. Namely the two groups quantified by ? that for the string 1.5.0a1.dev123457+nightly would match a1.dev123457+nightly and a1. I don't suspect this is an issue, but I wanted to make sure you were aware it still matches as an unnamed group since every other group is named.

.bumpversion.cfg Outdated Show resolved Hide resolved
@emmyoop
Copy link
Member Author

emmyoop commented Feb 2, 2023

@nathaniel-may

depending on how this matching is applied you may want to have start and stop anchors (^, $) to prevent matching in the middle of bad strings. e.g. - 1.2.3.4.5.6 matches twice: once on 1.2.3 and again on 4.5.6

bumpversion doesn't need this. If there is anything that falls outside the match it will fail. Tested here

There are also two unnamed groups. Namely the two groups quantified by ? that for the string 1.5.0a1.dev123457+nightly would match a1.dev123457+nightly and a1. I don't suspect this is an issue, but I wanted to make sure you were aware it still matches as an unnamed group since every other group is named.

It is not an issue. I've added a comment to clarify in the code.

@emmyoop emmyoop requested a review from nathaniel-may February 2, 2023 17:35
\.(?P<patch>[\d]+) # patch version number
(((?P<prekind>a|b|rc) # optional pre-release type
?(?P<num>[\d]+?)) # optional pre-release version number
\.?(?P<nightly>[a-z0-9]+\+[a-z]+)? # optional nightly release indicator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would need to give this some thought but I think we want 1.5.0a1 to be valid but 1.5.0a to be invalid right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the + in ?(?P<num>[\d]+?)) should handle that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed the parenthesis

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colin-rogers-dbt 1.5.0a is not valid because of the ? in the middle. it means what come before is optional, but if it's filled, what's after is required.
((?P<prekind>a|b|rc)?(?P<num>[\d]+?))

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

.bumpversion.cfg Outdated Show resolved Hide resolved
Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
@emmyoop emmyoop merged commit 2245d8d into main Feb 2, 2023
@emmyoop emmyoop deleted the er/fix-version-bump branch February 2, 2023 18:16
emmyoop added a commit that referenced this pull request Feb 2, 2023
* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg
emmyoop added a commit that referenced this pull request Feb 2, 2023
* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg
emmyoop added a commit that referenced this pull request Feb 2, 2023
* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg
emmyoop added a commit that referenced this pull request Feb 2, 2023
* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg
@iknox-fa iknox-fa restored the er/fix-version-bump branch February 3, 2023 16:25
emmyoop added a commit that referenced this pull request Feb 3, 2023
* [CI/CD] Update release workflow and introduce workflow for nightly releases (#6602)

* Add release workflows

* Update nightly-release.yml

* Set default `test_run` value to `true`

* Update .bumpversion.cfg

* Resolve review comment

- Update workflow docs
- Change workflow name
- Set `test_run` default value to `true`

* Update Slack secret

* PyPI

* Update release workflow (#6778)

- Update AWS secrets
- Rework condition for Slack notification

* update regex to match all iterations (#6839)

* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg

* put back correct version

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
emmyoop added a commit that referenced this pull request Feb 3, 2023
* [CI/CD] Update release workflow and introduce workflow for nightly releases (#6602)

* Add release workflows

* Update nightly-release.yml

* Set default `test_run` value to `true`

* Update .bumpversion.cfg

* Resolve review comment

- Update workflow docs
- Change workflow name
- Set `test_run` default value to `true`

* Update Slack secret

* PyPI

* Update release workflow (#6778)

- Update AWS secrets
- Rework condition for Slack notification

* update regex to match all iterations (#6839)

* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg

* put back correct version

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
emmyoop added a commit that referenced this pull request Feb 3, 2023
* [CI/CD] Update release workflow and introduce workflow for nightly releases (#6602)

* Add release workflows

* Update nightly-release.yml

* Set default `test_run` value to `true`

* Update .bumpversion.cfg

* Resolve review comment

- Update workflow docs
- Change workflow name
- Set `test_run` default value to `true`

* Update Slack secret

* PyPI

* Update release workflow (#6778)

- Update AWS secrets
- Rework condition for Slack notification

* update regex to match all iterations (#6839)

* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg

* put back correct version

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
emmyoop added a commit that referenced this pull request Feb 6, 2023
…6846)

* update regex to match all iterations (#6839)

* update regex to match all iterations

* convert to num to match all adapters

* add comments, remove extra .

* clarify with more comments

* Update .bumpversion.cfg

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

---------

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
# Conflicts:
#	.bumpversion.cfg

* put back correct version

* put back correct version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants