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

Remove markupsafe pin #5507

Merged
merged 3 commits into from
Jul 25, 2022
Merged

Remove markupsafe pin #5507

merged 3 commits into from
Jul 25, 2022

Conversation

emmyoop
Copy link
Member

@emmyoop emmyoop commented Jul 21, 2022

resolves #5506

Description

Remove pin of MarkUpSafe after Jinja2 upgrade.

Checklist

@emmyoop emmyoop requested a review from a team as a code owner July 21, 2022 14:30
@emmyoop emmyoop requested review from ChenyuLInx and stu-k July 21, 2022 14:30
@cla-bot cla-bot bot added the cla:yes label Jul 21, 2022
@github-actions
Copy link
Contributor

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.

core/setup.py Outdated
@@ -49,7 +49,7 @@
},
install_requires=[
"Jinja2==3.1.2",
"MarkupSafe>=0.23,<2.1",
"MarkupSafe>=2.0",
Copy link
Contributor

@jtcohen6 jtcohen6 Jul 21, 2022

Choose a reason for hiding this comment

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

Any sense in just removing it entirely?

Reasoning:

  • It's a second-order dependency of Jinja
  • We didn't have it here until we needed to add the upper boundary earlier this year (Pin MarkupSafe==2.0.1 #4746)
  • We should move in the direction of minimal dependency specification in setup.py, full dependency specification (exact versions tested + guaranteed to work) in requirements.txt

Copy link
Member Author

@emmyoop emmyoop Jul 21, 2022

Choose a reason for hiding this comment

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

Removing the pin entirely is the right move. I wasn't aware of the history there.

It looks like dbt-labs/dbt-server(#48) and dbt-labs/dbt-cloud (#4928) both pinned it as well and can possibly remove their pins.

@emmyoop emmyoop changed the title update markupsafe pin Remove markupsafe pin Jul 21, 2022
@emmyoop emmyoop requested a review from jtcohen6 July 21, 2022 16:27
@emmyoop emmyoop merged commit 3f54f30 into main Jul 25, 2022
@emmyoop emmyoop deleted the er/ct-876-markupsafe branch July 25, 2022 14:47
@MichaelTiemannOSC
Copy link

This missed the 1.2.1 dbt-core cutoff. Would be so nice if it could land in a near-term 1.2.2.

@emmyoop
Copy link
Member Author

emmyoop commented Aug 28, 2022

@MichaelTiemannOSC unfortunately we will not be able to remove this pin for dbt versions previous to 1.3.0.

In dbt-core 1.3.0 we were able to upgrade Jinja2 to version 3 which is what allowed the removal of the Markupsafe pin. You can see more information on the dependency conflicts between markupsafe and Jinja2 in the related issue.

agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
* update markupsafe pin

* add changelog

* completely remove markupsafe pin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants