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

[AIRFLOW-7067] Pinned version of Apache Airflow #7730

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Mar 15, 2020


Issue link: AIRFLOW-7067

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


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.
Read the Pull Request Guidelines for more information.

@codecov-io
Copy link

codecov-io commented Mar 15, 2020

Codecov Report

Merging #7730 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7730      +/-   ##
==========================================
+ Coverage   86.88%   86.92%   +0.03%     
==========================================
  Files         924      924              
  Lines       44674    44792     +118     
==========================================
+ Hits        38816    38934     +118     
  Misses       5858     5858              
Impacted Files Coverage Δ
airflow/bin/cli.py 97.66% <0.00%> (+2.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ad1844...6a80f5f. Read the comment docs.

@potiuk potiuk force-pushed the AIRFLOW-7067-pinned-version-of-airflow branch 4 times, most recently from c6a3d1a to ff4bfcd Compare March 15, 2020 12:59
INSTALL Outdated Show resolved Hide resolved
INSTALL Outdated Show resolved Hide resolved
@potiuk
Copy link
Member Author

potiuk commented Mar 15, 2020

BTW. I am going to simplify it even more - no need to store fixed requirements in setup.py

@potiuk potiuk force-pushed the AIRFLOW-7067-pinned-version-of-airflow branch 2 times, most recently from 54f91ca to bfd5d21 Compare March 15, 2020 16:51
@potiuk
Copy link
Member Author

potiuk commented Mar 15, 2020

All should be update and much nicer now @ashb !

@potiuk
Copy link
Member Author

potiuk commented Mar 15, 2020

And all green!

@potiuk potiuk force-pushed the AIRFLOW-7067-pinned-version-of-airflow branch 8 times, most recently from 3b96d22 to 61b8f3d Compare March 22, 2020 10:15
INSTALL Outdated

brew install sqlite mysql postgresql

# [required] fetch the tarball and untar the source chnge into the directory that was untarred.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# [required] fetch the tarball and untar the source chnge into the directory that was untarred.
# [required] fetch the tarball and untar the source, move into the directory that was untarred.

# You can also install recommended version of the dependencies by using requirements.txt
# as constraint file

pip install . --constraint requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

How should I know if I should use requirements or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

You do not have to - it's the recommended version of dependencies. This is just a hint that you can do it. I proposed to have "apache-airflow-pinned" package release separately. with clear purpose in mind "to have repeatable installation"

I agreed having the --constraint and having to know which version to install might be confusing - that was my point in the discussion - but there are different opinions. For now I just want to merge requirements.txt and it's maintenance mechanism so that we avoid further breakage. But the discussion continues here how to approach it:

https://lists.apache.org/thread.html/rf8b0ea9a7b93482734ae9e10e7d87f3c31679edb5eaf88496e733aa5%40%3Cdev.airflow.apache.org%3E

I invite you to take part in the discussion and propose some solutions/approach. I would really love to have yours (and others) opinions in the thread there.

@potiuk potiuk force-pushed the AIRFLOW-7067-pinned-version-of-airflow branch from 61b8f3d to 6a80f5f Compare March 22, 2020 10:58
@potiuk potiuk merged commit 8c56388 into apache:master Mar 22, 2020
potiuk added a commit that referenced this pull request Mar 28, 2020
kaxil pushed a commit to astronomer/airflow that referenced this pull request Mar 30, 2020
kaxil pushed a commit that referenced this pull request Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants