Skip to content

Modernizes usage of PIP in Airflow images - #20678

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:modernize-pip-usage-in-images
Closed

Modernizes usage of PIP in Airflow images#20678
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:modernize-pip-usage-in-images

Conversation

@potiuk

@potiuk potiuk commented Jan 5, 2022

Copy link
Copy Markdown
Member
  • removes PIP_INSTALL_USER variable
  • upgrades PIP to 21.3.1
  • removes AIRFLOW_INSTALL_USER_FLAG as it is not needed
  • removes spurious usage of --upgrade flag for PIP
  • adds better diagnostics during the build for PIP location and version

Separated out from #20238

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg Bot added area:dev-tools area:production-image Production image improvements and fixes kind:documentation labels Jan 5, 2022
@potiuk
potiuk requested a review from uranusjr January 5, 2022 11:43
@potiuk
potiuk force-pushed the modernize-pip-usage-in-images branch from 6718b6d to 4fd118f Compare January 5, 2022 11:46
@potiuk potiuk mentioned this pull request Jan 5, 2022
Merged
@potiuk
potiuk force-pushed the modernize-pip-usage-in-images branch from 4fd118f to 80b3b48 Compare January 5, 2022 12:06
uranusjr
uranusjr reviewed Jan 5, 2022
View reviewed changes

@uranusjr uranusjr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mostly lgtm, just some very minor questions.

Comment thread dev/README_RELEASE_PROVIDER_PACKAGES.md Outdated
Comment thread scripts/docker/common.sh Outdated
Comment thread scripts/docker/install_airflow.sh Outdated
Comment thread scripts/docker/install_pip_version.sh Outdated
@potiuk
potiuk force-pushed the modernize-pip-usage-in-images branch from 80b3b48 to 113ca34 Compare January 5, 2022 12:29
@potiuk

potiuk commented Jan 5, 2022

Copy link
Copy Markdown
Member Author

All should be addressed.

@potiuk potiuk closed this Jan 5, 2022
@potiuk potiuk reopened this Jan 5, 2022
ashb
ashb reviewed Jan 5, 2022
View reviewed changes
Comment thread Dockerfile Outdated
ashb
ashb reviewed Jan 5, 2022
View reviewed changes
Comment thread scripts/docker/install_additional_dependencies.sh Outdated
@potiuk
potiuk force-pushed the modernize-pip-usage-in-images branch from 113ca34 to 21cb191 Compare January 5, 2022 16:40
@potiuk
Modernizes usage of PIP in Airflow images
fefcf7b 
* removes PIP_INSTALL_USER variable
* upgrades PIP to 21.3.1
* removes AIRFLOW_INSTALL_USER_FLAG as it is not needed
* removes spurious usage of --upgrade flag for PIP
* adds better diagnostics during the build for PIP location and version

Separated out from apache#20238
@potiuk
potiuk force-pushed the modernize-pip-usage-in-images branch from 21cb191 to fefcf7b Compare January 5, 2022 16:45
@potiuk

potiuk commented Jan 5, 2022

Copy link
Copy Markdown
Member Author

Addressed.

@potiuk potiuk closed this Jan 5, 2022
@potiuk potiuk reopened this Jan 5, 2022
@github-actions

github-actions Bot commented Jan 6, 2022

Copy link
Copy Markdown
Contributor

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 Jan 6, 2022
@uranusjr

uranusjr commented Jan 6, 2022

Copy link
Copy Markdown
Member

Covered by #20679.

@uranusjr uranusjr closed this Jan 6, 2022
@potiuk

potiuk commented Jan 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

For the future - @uranusjr - > I wanted to merge those two separately. They contained a lot of context in the "commit messages" and i separated them out deliberately (and planned to merge #20678 first and rebase #20679 on top). By squashing those two - we lost the context in the commit messages and all the changes were squashed :( .

@potiuk

potiuk commented Jan 6, 2022

Copy link
Copy Markdown
Member Author

We use to (and I did in #20679) write this kind of message when split out bigger work into separate commits: Based on #20678 - so please take a look at the last commit only.

@potiuk potiuk mentioned this pull request Jan 6, 2022
Merged
@potiuk
potiuk deleted the modernize-pip-usage-in-images branch July 29, 2022 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashb ashb ashb left review comments
@uranusjr uranusjr uranusjr approved these changes
@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner
@kaxil kaxil Awaiting requested review from kaxil
@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees

No one assigned

Labels

area:dev-tools area:production-image Production image improvements and fixes full tests needed We need to run full set of tests for this PR to merge kind:documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@potiuk @uranusjr @ashb