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

Flexibilize MarkupSafe pinned version #5039

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

adamantike
Copy link
Contributor

@adamantike adamantike commented Apr 12, 2022

Description

The current MarkupSafe pinned version has been added in #4746 as a temporary fix for #4745.

However, the current restrictive approach isn't compatible with other libraries that could require an even older version of MarkupSafe, like Airflow 2.2.2 [0], which requires markupsafe>=1.1.1, <2.0.

To avoid that issue, we can allow a greater range of supported MarkupSafe versions. Considering the direct dependency dbt-core has is Jinja2==2.11.3, we can use its pinning as the lower bound, which is MarkupSafe>=0.23 [1].

This fix should be also backported this to 1.0.latest for inclusion in the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have added information about my change to be included in the CHANGELOG.

The current `MarkupSafe` pinned version has been added in dbt-labs#4746 as a
temporary fix for dbt-labs#4745.

However, the current restrictive approach isn't compatible with other
libraries that could require an even older version of `MarkupSafe`, like
Airflow `2.2.2` [0], which requires `markupsafe>=1.1.1, <2.0`.

To avoid that issue, we can allow a greater range of supported
`MarkupSafe` versions. Considering the direct dependency `dbt-core` has
is `Jinja2==2.11.3`, we can use its pinning as the lower bound, which is
`MarkupSafe>=0.23` [1].

This fix should be also backported this to `1.0.latest` for inclusion in
the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53
@cla-bot cla-bot bot added the cla:yes label Apr 12, 2022
@adamantike adamantike marked this pull request as ready for review April 12, 2022 15:41
@adamantike adamantike requested a review from a team as a code owner April 12, 2022 15:41
@adamantike
Copy link
Contributor Author

Not sure if a change like this one would require a changelog entry. Open to suggestions!

@iknox-fa
Copy link
Contributor

@adamantike Thanks for your PR.. a changelog entry for this would be great!

Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

LGTM

@adamantike
Copy link
Contributor Author

Thanks for the quick reply! So, to validate before adding it to the changelog, would this be applicable to be backported to 1.0.x?

In case it is, should I add the changelog entry for that here too, or do you take care of that when cherry-picking the change?

@ChenyuLInx
Copy link
Contributor

@adamantike we changed our changelog system to changie so all you need to do is follow the instruction in the link and the actual changelog.md will be generated during release.
@iknox-fa is there a reason we are fixing the upper bound version of MarkUpSafe we can use? I think the install_requires is usually only used for lower bound unless there's a good reason to cap the version.

@adamantike
Copy link
Contributor Author

Thanks, I have added the changelog entry after following the changie instructions.

Regarding why we need to provide an upper bound, it's because of this discussion: pallets/jinja#1602. jinja didn't fix it in their side because version 2.x isn't supported anymore, so dbt-core requires this pinning until the jinja2 dependency can be upgraded to version 3.x.

@ChenyuLInx ChenyuLInx merged commit 5ff81c2 into dbt-labs:main Apr 13, 2022
@ChenyuLInx
Copy link
Contributor

@adamantike Thanks for contributing!!!

@adamantike adamantike deleted the flexibilize-markupsafe-fix branch April 13, 2022 19:54
@adamantike
Copy link
Contributor Author

Thanks for the quick resolution!

With this PR merged, is there a formal process to ask for this fix to be backported to 1.0.latest?

VersusFacit pushed a commit that referenced this pull request Apr 14, 2022
* Flexibilize MarkupSafe pinned version

The current `MarkupSafe` pinned version has been added in #4746 as a
temporary fix for #4745.

However, the current restrictive approach isn't compatible with other
libraries that could require an even older version of `MarkupSafe`, like
Airflow `2.2.2` [0], which requires `markupsafe>=1.1.1, <2.0`.

To avoid that issue, we can allow a greater range of supported
`MarkupSafe` versions. Considering the direct dependency `dbt-core` has
is `Jinja2==2.11.3`, we can use its pinning as the lower bound, which is
`MarkupSafe>=0.23` [1].

This fix should be also backported this to `1.0.latest` for inclusion in
the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53
@stu-k
Copy link
Contributor

stu-k commented Apr 14, 2022

@adamantike Thanks again for the contribution! Our team will handle backporting your change!

@leahwicz leahwicz added backport 1.0.latest Tag for PR to be backported to the 1.0.latest branch backport 1.1.latest and removed backport 1.0.latest Tag for PR to be backported to the 1.0.latest branch backport 1.1.latest labels Apr 14, 2022
leahwicz pushed a commit that referenced this pull request Apr 18, 2022
* Flexibilize MarkupSafe pinned version

The current `MarkupSafe` pinned version has been added in #4746 as a
temporary fix for #4745.

However, the current restrictive approach isn't compatible with other
libraries that could require an even older version of `MarkupSafe`, like
Airflow `2.2.2` [0], which requires `markupsafe>=1.1.1, <2.0`.

To avoid that issue, we can allow a greater range of supported
`MarkupSafe` versions. Considering the direct dependency `dbt-core` has
is `Jinja2==2.11.3`, we can use its pinning as the lower bound, which is
`MarkupSafe>=0.23` [1].

This fix should be also backported this to `1.0.latest` for inclusion in
the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53
leahwicz pushed a commit that referenced this pull request Apr 19, 2022
* Flexibilize MarkupSafe pinned version

The current `MarkupSafe` pinned version has been added in #4746 as a
temporary fix for #4745.

However, the current restrictive approach isn't compatible with other
libraries that could require an even older version of `MarkupSafe`, like
Airflow `2.2.2` [0], which requires `markupsafe>=1.1.1, <2.0`.

To avoid that issue, we can allow a greater range of supported
`MarkupSafe` versions. Considering the direct dependency `dbt-core` has
is `Jinja2==2.11.3`, we can use its pinning as the lower bound, which is
`MarkupSafe>=0.23` [1].

This fix should be also backported this to `1.0.latest` for inclusion in
the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53
leahwicz added a commit that referenced this pull request Apr 19, 2022
* Flexibilize MarkupSafe pinned version

The current `MarkupSafe` pinned version has been added in #4746 as a
temporary fix for #4745.

However, the current restrictive approach isn't compatible with other
libraries that could require an even older version of `MarkupSafe`, like
Airflow `2.2.2` [0], which requires `markupsafe>=1.1.1, <2.0`.

To avoid that issue, we can allow a greater range of supported
`MarkupSafe` versions. Considering the direct dependency `dbt-core` has
is `Jinja2==2.11.3`, we can use its pinning as the lower bound, which is
`MarkupSafe>=0.23` [1].

This fix should be also backported this to `1.0.latest` for inclusion in
the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53

Co-authored-by: Michael Manganiello <adamantike@users.noreply.github.com>
leahwicz added a commit that referenced this pull request May 9, 2022
* Flexibilize MarkupSafe pinned version

The current `MarkupSafe` pinned version has been added in #4746 as a
temporary fix for #4745.

However, the current restrictive approach isn't compatible with other
libraries that could require an even older version of `MarkupSafe`, like
Airflow `2.2.2` [0], which requires `markupsafe>=1.1.1, <2.0`.

To avoid that issue, we can allow a greater range of supported
`MarkupSafe` versions. Considering the direct dependency `dbt-core` has
is `Jinja2==2.11.3`, we can use its pinning as the lower bound, which is
`MarkupSafe>=0.23` [1].

This fix should be also backported this to `1.0.latest` for inclusion in
the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53

Co-authored-by: Michael Manganiello <adamantike@users.noreply.github.com>
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request May 20, 2022
* Flexibilize MarkupSafe pinned version

The current `MarkupSafe` pinned version has been added in dbt-labs#4746 as a
temporary fix for dbt-labs#4745.

However, the current restrictive approach isn't compatible with other
libraries that could require an even older version of `MarkupSafe`, like
Airflow `2.2.2` [0], which requires `markupsafe>=1.1.1, <2.0`.

To avoid that issue, we can allow a greater range of supported
`MarkupSafe` versions. Considering the direct dependency `dbt-core` has
is `Jinja2==2.11.3`, we can use its pinning as the lower bound, which is
`MarkupSafe>=0.23` [1].

This fix should be also backported this to `1.0.latest` for inclusion in
the next v1.0 patch.

[0] https://github.com/adamantike/airflow/blob/2.2.2/setup.cfg#L125
[1] https://github.com/pallets/jinja/blob/2.11.3/setup.py#L53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.0.latest Tag for PR to be backported to the 1.0.latest branch backport 1.1.latest cla:yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants