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

Allow greater mashumaro versions #7294

Merged
merged 3 commits into from
Apr 12, 2023
Merged

Allow greater mashumaro versions #7294

merged 3 commits into from
Apr 12, 2023

Conversation

benallard
Copy link
Contributor

@benallard benallard commented Apr 7, 2023

Current 3.5 is working fine.

resolves #7295

Description

Checklist

Current 3.5 is working fine.
@benallard benallard requested a review from a team as a code owner April 7, 2023 14:42
@benallard benallard requested review from gshank and dbeatty10 April 7, 2023 14:42
@cla-bot
Copy link

cla-bot bot commented Apr 7, 2023

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @benallard

@dbeatty10 dbeatty10 added the dependencies Changes to the version of dbt dependencies label Apr 7, 2023
@dbeatty10
Copy link
Contributor

@gshank I briefly discussed this with @emmyoop offline. She suspects we have mashumaro pinned because upgrades have broken us in the past.

How do you think we should proceed?

  1. Keep hard-pin
  2. Expand upper-bound to highest version known to work
  3. Something else

@gshank
Copy link
Contributor

gshank commented Apr 7, 2023

There have been multiple releases of mashumaro that have broken our code. Which means that even a range could be problematic because there could be a version in the range that won't work. We'd have to test with each version in the range...

I'm inclined to hard pin it.

@benallard Is there some reason you're requesting this? Are you using mashumaro outside of dbt?

@benallard
Copy link
Contributor Author

@gsank, I'm packaging dbt for AUR (arch), so we have to rely on the current packaged mashumaro version. The tests worked fine, so I thought that wouldn't be too much of an issue.

If there has been breakage in the past, I hope they were documented as test, so we can make sure they don't occur again. That way, we could rely on a relax requirement + working test. Any packager will let the test run within its own environment, so version requirement in the setup.py are in my eyes more like a hint on already tested tested version.

Should I better open an issue (which I'll have to do anyway, it seems), to discuss this further, then we can discard this pull-request.

@benallard
Copy link
Contributor Author

See #7295

@jtcohen6
Copy link
Contributor

For the reasons @gshank described above, we're going to keep mashumaro pinned to a specific release for now. However, I would be supportive of bumping the pinned version to the latest, if you want to update this PR to mashumaro==3.6 (just released!).

@dbeatty10
Copy link
Contributor

@benallard We require contributors to sign our Contributor License Agreement -- would you be willing to sign that so we can give you credit for this contribution?

@cla-bot cla-bot bot added the cla:yes label Apr 11, 2023
@benallard
Copy link
Contributor Author

CLA ✅
3.6 ✅

@jtcohen6
Copy link
Contributor

@benallard Just missing a changelog entry:

$ changie new
✔ Dependencies
Body: Bump mashumaro[msgpack] from 3.3.1 to 3.6
GitHub Username(s) (separated by a single space if multiple): benallard
GitHub Pull Request Number (separated by a single space if multiple): 7294

@benallard
Copy link
Contributor Author

Yeah, changie ✅

@gshank gshank merged commit c3230d3 into dbt-labs:main Apr 12, 2023
@benallard benallard deleted the patch-1 branch April 12, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes dependencies Changes to the version of dbt dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2382] [Feature] allow a broader list of mashumaro versions.
5 participants