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

CT 2000 fix semver prerelease comparisons #6838

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Feb 1, 2023

resolves #6834
resolves #6461

Description

In semver.py we were using packaging.version.parse to parse the prerelease portion of versions, but since that version parsing is incompatible with SemVer, it started failing once the LegacyVersion fallback was removed.

This removes the dependency on packaging and subsitutes some comparison code specifically for prepreleases, so that we can remove the pin on packaging.

Checklist

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

Highlights

  • 🎉 This looks like it does as described and removes the dependency on packaging.version.parse()
  • 💡 It seems like we should still swing back later and try to reduce our need for custom PEP 440 and SemVer parsing and comparison logic

More detail

I double-checked that CI indeed used packaging>=23.0:
https://github.com/dbt-labs/dbt-core/actions/runs/4068580210/jobs/7007304928#step:4:55

I gave a reading of the logic in compare() and _nat_cmp() and cmp_prerelease_tag(), and I didn't notice anything amiss or a key case not covered.

My only nits are the variable names prcmp (for "prerelease comparison"?) and _nat_cmp (for "natural comparison"?) felt a little terse and not 100% obvious. But given the time-sensitive nature of your fixes here, these didn't feel quite important enough to block or even raise as formal suggestions.

✅ Ultimately, it looks like the key test cases are covered by test__filter_installable.

Side note: I'm guessing black doesn't run against code in the test directory, and I won't raise any nits about white space formatting within test/unit/test_semver.py.

@gshank
Copy link
Contributor Author

gshank commented Feb 2, 2023

We should definitely look at a more consistent solution to these version problems, and look into using some existing package.

@gshank gshank merged commit d9424cc into main Feb 2, 2023
@gshank gshank deleted the ct-2000-fix_semver_prerelease_comparisons branch February 2, 2023 17:22
@iknox-fa iknox-fa restored the ct-2000-fix_semver_prerelease_comparisons branch February 3, 2023 16:24
github-actions bot pushed a commit that referenced this pull request Feb 13, 2023
* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)
@jtcohen6
Copy link
Contributor

Let's include this in the next v1.4.x patch release as well, since users will continue to have issues installing dbt-core with the tight pin. The change was small & precise, and fixed a narrow bug (the previous logic was genuinely incorrect).

jtcohen6 pushed a commit that referenced this pull request Feb 13, 2023
* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>
gshank added a commit that referenced this pull request Sep 8, 2024
* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>
gshank added a commit that referenced this pull request Sep 9, 2024
* CT 2000 fix semver prerelease comparisons (#6838) (#6958)

* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>

* Fix regression in semver comparison logic (#7040) (#7041)

(cherry picked from commit 915585c)

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>

* Fix changed name of exception

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
github-actions bot pushed a commit that referenced this pull request Sep 9, 2024
* CT 2000 fix semver prerelease comparisons (#6838) (#6958)

* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>

* Fix regression in semver comparison logic (#7040) (#7041)

(cherry picked from commit 915585c)

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>

* Fix changed name of exception

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
(cherry picked from commit 9e85a22)
gshank added a commit that referenced this pull request Sep 9, 2024
* CT 2000 fix semver prerelease comparisons (#6838) (#6958)

* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>

* Fix regression in semver comparison logic (#7040) (#7041)

(cherry picked from commit 915585c)

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>

* Fix changed name of exception

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
gshank added a commit that referenced this pull request Sep 9, 2024
* CT 2000 fix semver prerelease comparisons (#6838) (#6958)

* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>

* Fix regression in semver comparison logic (#7040) (#7041)

(cherry picked from commit 915585c)

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>

* Fix changed name of exception

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
gshank added a commit that referenced this pull request Sep 10, 2024
* CT 2000 fix semver prerelease comparisons (#6838) (#6958)

* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>

* Fix regression in semver comparison logic (#7040) (#7041)

(cherry picked from commit 915585c)

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>

* Fix changed name of exception

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
(cherry picked from commit 9e85a22)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>
gshank added a commit that referenced this pull request Sep 10, 2024
* CT 2000 fix semver prerelease comparisons (#6838) (#6958)

* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)



* Fix regression in semver comparison logic (#7040) (#7041)

(cherry picked from commit 915585c)



* Fix changed name of exception

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
gshank added a commit that referenced this pull request Sep 10, 2024
* CT 2000 fix semver prerelease comparisons (#6838) (#6958)

* Modify semver.py to not use packaging.version.parse

* Changie

(cherry picked from commit d9424cc)



* Fix regression in semver comparison logic (#7040) (#7041)

(cherry picked from commit 915585c)



* Fix changed name of exception

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2000] Fix broken semver comparison logic [CT-1688] [Feature] Remove packaging <22 version cap
3 participants