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

Lift off upper bound for MarkupSafe #20113

Merged
merged 1 commit into from Dec 8, 2021
Merged

Lift off upper bound for MarkupSafe #20113

merged 1 commit into from Dec 8, 2021

Conversation

tsingh2k15
Copy link
Contributor

Per discussion and guidance from #19753, opening this PR for review. Based on if all the tests pass, this could be reviewed further. Resolves #19761.

Per discussion and guidance from apache#19753, opening this PR for review. Based on if all the tests pass, this could be reviewed further. Resolves apache#19761.
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Everything seems to work well!

@github-actions
Copy link

github-actions bot commented Dec 7, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 7, 2021
@tsingh2k15
Copy link
Contributor Author

tsingh2k15 commented Dec 7, 2021

Everything seems to work well!

Thank you for the review @uranusjr
When this PR is merged to main, where could I find a constraint file to start using this change in my installation scripts? I am curious to understand when the constraint file would be available or how I can start using this change. Do I need to wait for a release?

@potiuk potiuk merged commit bcacc51 into apache:main Dec 8, 2021
@potiuk
Copy link
Member

potiuk commented Dec 8, 2021

Hey @tsingh2k15 - all looks good. In the "Build Images" job you can find that this build actually used 2.0.1 version:

https://github.com/apache/airflow/runs/4448089223?check_suite_focus=true#step:8:713

    Downloading MarkupSafe-2.0.1-cp37-cp37m-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_12_x86_64.manylinux2010_x86_64.whl (31 kB)

I merged it.

When this PR is merged to main, where could I find a constraint file to start using this change in my installation scripts? I am curious to understand when the constraint file would be available or how I can start using this change. Do I need to wait for a release?

When the PR is merged, the constraints will be automatically updated in the "constraint-main" branch of our repo - but ONLY if all tests pass (we have now WAY more stable build so this is likely to happen tonight). Also if there are many subsequent merges one after another, the build might get cancelled due to "concurrency" so sometimes it will take some time for the "green" build to push such new constraints.

You can see the history of those constraint changes here: https://github.com/apache/airflow/commits/constraints-main

Once there you will be able to use it as described in https://airflow.apache.org/docs/apache-airflow/stable/installation/installing-from-pypi.html but use "constraints-main" rather than "constaints-2.2.2" as version.

But if you want to check it yourself it's very easy to download image from your PR build (we have all the images we used for CI in our registry) and run it locally with Breeze. The below should pull your image and start airlfow (loading default connections and example dags for easier testing). It should bring you a running airflow instance with webserver available at http://locahost:28080 (admin/admin):

./breeze start-airflow --github-image-id 13b0af1a2211f3e905d23bc5e1808307d577183d --load-example-dags --load-default-connections

(The image id you can find in logs of your build)

@potiuk potiuk added this to the Airflow 2.2.3 milestone Dec 8, 2021
@potiuk
Copy link
Member

potiuk commented Dec 8, 2021

But if you want to start using it for your own builds, you will need to wait for new release unfortunately as this is released airlfow package dependency (MarkupSafe < 2.0.0) and PIP will refuse to install this version with released airflow. Potentially, you could install the package with ignoring the deps I think, but I do not think it is a good idea.

I marked both PRs to be integrated in 2.2.3 so it should be soon-ish (before/around Xmas I believe),

jedcunningham pushed a commit that referenced this pull request Dec 8, 2021
Per discussion and guidance from #19753, opening this PR for review. Based on if all the tests pass, this could be reviewed further. Resolves #19761.

(cherry picked from commit bcacc51)
@tsingh2k15
Copy link
Contributor Author

This makes sense, thanks for the clarification!

@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Dec 8, 2021
@potiuk
Copy link
Member

potiuk commented Dec 8, 2021

Oh yeah - as predicted : bb48bc3

@potiuk potiuk modified the milestones: Airflow 2.2.3, Airflow 2.2.4 Jan 22, 2022
potiuk pushed a commit that referenced this pull request Jan 22, 2022
Per discussion and guidance from #19753, opening this PR for review. Based on if all the tests pass, this could be reviewed further. Resolves #19761.

(cherry picked from commit bcacc51)
jedcunningham pushed a commit that referenced this pull request Jan 27, 2022
Per discussion and guidance from #19753, opening this PR for review. Based on if all the tests pass, this could be reviewed further. Resolves #19761.

(cherry picked from commit bcacc51)
@jedcunningham
Copy link
Member

(Kicking this back to the 2.2.3 milestone as that's when this landed in a release)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

removing upper bound for markupsafe
4 participants