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.

Sign up for GitHub

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

Jump to bottom

[AIRFLOW-4062] Clear install extra package command #4897

Merged
merged 1 commit into from
Mar 24, 2019
Merged

[AIRFLOW-4062] Clear install extra package command #4897

merged 1 commit into from
Mar 24, 2019

Conversation

zhongjiajie
Copy link
Member

Some command for installing extra packages are
pip install apache-airflow[devel] we should
clear install extra package command to
pip install 'apache-airflow[devel]'

[ci skip]

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-4062], code changes always need a Jira issue.

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Some command for installing extra packages are
pip install apache-airflow[devel] we should
clear install extra package command to
pip install 'apache-airflow[devel]'

@zhongjiajie
Copy link
Member Author

I create jira ticket becase of change log code in https://github.com/apache/airflow/pull/4897/files#diff-e90ff8ca3f03b5d363736e93748ef634R67

@mik-laj
Copy link
Member

mik-laj commented Mar 11, 2019

Why is it important?

@zhongjiajie
Copy link
Member Author

PTAL @XD-DENG @feng-tao @Fokko

@zhongjiajie
Copy link
Member Author

@mik-laj pip install apache-airflow[devel] will get error in ubuntu, but pip install 'apache-airflow[devel]' will not

@XD-DENG
Copy link
Member

XD-DENG commented Mar 11, 2019

Hi @zhongjiajie , I tried to run pip install apache-airflow[devel] on Ubuntu, and there was no issue/error.

Python: Python 2.7.15rc1
Pip: pip 9.0.1 from /usr/lib/python2.7/dist-packages (python 2.7)
Ubuntu: Linux **** #34-Ubuntu SMP Thu Jan 17 15:18:09 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux (an AWS instance)

Please double-check from your side.

@XD-DENG XD-DENG added the Can't Reproduce The problem cannot be reproduced label Mar 11, 2019
@zhongjiajie
Copy link
Member Author

@XD-DENG In my side, it raise error.
pic

@zhongjiajie
Copy link
Member Author

@XD-DENG I get it, It is zsh, according to https://stackoverflow.com/a/30539963/7152658. I don't know in this situation I should close PR or not.

@XD-DENG
Copy link
Member

XD-DENG commented Mar 11, 2019

I would suggest closing it as I don't see it as an issue/bug.

@zhongjiajie
Copy link
Member Author

@XD-DENG Think about it for a while, I want to discuss PR once again. I have two points.

  • We should compatible our command in many operating system and many type of shell, and this change will not break others and could use in zsh
  • We have some other pip install using '' between extra package, just like below.

https://github.com/apache/airflow/blame/master/docs/howto/executor/use-celery.rst#L64
https://github.com/apache/airflow/blame/master/CONTRIBUTING.md#L132

7yl4r
7yl4r approved these changes Mar 11, 2019
View reviewed changes
Copy link

@7yl4r 7yl4r left a comment

Choose a reason for hiding this comment

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

Adding single quotes to pip install argument fixes install for zsh and doesn't hurt other shells.
Sure, why not?

@Fokko
Copy link
Contributor

Fokko commented Mar 23, 2019

I've seen people make this mistake often. I'll would like to get this in. @zhongjiajie. Can you rebase?

@kaxil
Copy link
Member

kaxil commented Mar 23, 2019

I agree with Fokko

@zhongjiajie
[AIRFLOW-4062] Clear install extra package command
119f387 
Some command for installing extra packages are
`pip install apache-airflow[devel]` we should
clear install extra package command to
`pip install 'apache-airflow[devel]'`

[ci skip]
@zhongjiajie zhongjiajie force-pushed the docs_clear_install_package_command branch from 4402415 to 119f387 Compare March 24, 2019 07:33
@zhongjiajie
Copy link
Member Author

@Fokko I rebase on master, PTAL, thanks.

@ashb ashb merged commit d4655c5 into apache:master Mar 24, 2019
ashb added a commit that referenced this pull request Mar 24, 2019
@ashb
Revert "[AIRFLOW-4062] Improve docs on install extra package commands (…
97ca368 
…#4897)"

This reverts commit d4655c5.
@ashb ashb mentioned this pull request Mar 24, 2019
Merged
ashb added a commit that referenced this pull request Mar 24, 2019
@ashb
Revert "[AIRFLOW-4062] Improve docs on install extra package commands (…
c159e8e 
…#4897)" (#4965)

This reverts commit d4655c5 as it causes doc test warnings/failures.
@ashb
Copy link
Member

ashb commented Mar 24, 2019

@zhongjiajie This causes test failures due to invalid docs. Turns out CI skip was a bad idea on docs, at least in this case :D

We can have this back once the test pass :)

@zhongjiajie
Copy link
Member Author

@ashb I am so sorry about that. I will fix that.

@zhongjiajie zhongjiajie mentioned this pull request Mar 24, 2019
Merged
1 task
cthenderson pushed a commit to cthenderson/apache-airflow that referenced this pull request Apr 16, 2019
@zhongjiajie
[AIRFLOW-4062] Improve docs on install extra package commands (apache…
99921e0 
…#4897)

Some command for installing extra packages are
`pip install apache-airflow[devel]` we should
clear install extra package command to
`pip install 'apache-airflow[devel]'`

[ci skip]
cthenderson pushed a commit to cthenderson/apache-airflow that referenced this pull request Apr 16, 2019
@ashb
Revert "[AIRFLOW-4062] Improve docs on install extra package commands (…
248926a 
…apache#4897)" (apache#4965)

This reverts commit d4655c5 as it causes doc test warnings/failures.
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
@zhongjiajie @andriisoldatenko
[AIRFLOW-4062] Improve docs on install extra package commands (apache…
2f6c926 
…#4897)

Some command for installing extra packages are
`pip install apache-airflow[devel]` we should
clear install extra package command to
`pip install 'apache-airflow[devel]'`

[ci skip]
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
@ashb @andriisoldatenko
Revert "[AIRFLOW-4062] Improve docs on install extra package commands (…
be096eb 
…apache#4897)" (apache#4965)

This reverts commit d4655c5 as it causes doc test warnings/failures.
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
@zhongjiajie
[AIRFLOW-4062] Improve docs on install extra package commands (apache…
02c9742 
…#4897)

Some command for installing extra packages are
`pip install apache-airflow[devel]` we should
clear install extra package command to
`pip install 'apache-airflow[devel]'`

[ci skip]
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
@ashb
Revert "[AIRFLOW-4062] Improve docs on install extra package commands (…
2ce5285 
…apache#4897)" (apache#4965)

This reverts commit d4655c5 as it causes doc test warnings/failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7yl4r 7yl4r 7yl4r approved these changes

Assignees
No one assigned
Labels
Can't Reproduce The problem cannot be reproduced
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zhongjiajie @mik-laj @XD-DENG @Fokko @kaxil @ashb @7yl4r