Skip to content

[AIRFLOW-3707] Group subpackages by cloud providers#4524

Merged
ashb merged 1 commit intoapache:masterfrom
mik-laj:merge-cloud-providers
Feb 8, 2019
Merged

[AIRFLOW-3707] Group subpackages by cloud providers#4524
ashb merged 1 commit intoapache:masterfrom
mik-laj:merge-cloud-providers

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Jan 14, 2019

Make sure you have checked all steps below.

Jira

Description

I propose to group sub packages by. cloud providers. There is a good chance that if the user needs one package of the given clouds, he will need more in a moment. This will be more than consistent with the way the gcp_api package works.

Tests

  • No tests

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • Updated docs

Code Quality

  • Passes flake8

@mik-laj
Copy link
Member Author

mik-laj commented Jan 14, 2019

@ashb Recently, you gave me a thumb up on a comment that pointed to this problem.

@mik-laj mik-laj force-pushed the merge-cloud-providers branch from 67b27f6 to 846a5b8 Compare January 14, 2019 20:54
@codecov-io
Copy link

codecov-io commented Jan 14, 2019

Codecov Report

Merging #4524 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4524      +/-   ##
==========================================
- Coverage   74.35%   74.34%   -0.01%     
==========================================
  Files         428      428              
  Lines       27947    27947              
==========================================
- Hits        20779    20778       -1     
- Misses       7168     7169       +1
Impacted Files Coverage Δ
airflow/utils/log/s3_task_handler.py 98.57% <ø> (ø) ⬆️
airflow/models/__init__.py 92.81% <0%> (-0.05%) ⬇️

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 e607106...8e7bcab. Read the comment docs.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

This needs a note in updating, otherwise someone might try to install apache-airflow[s3] and be confused by it not working.

It might be nice to maintain the old names in the extra section for a release or two to ease upgrade too?

@mik-laj
Copy link
Member Author

mik-laj commented Jan 14, 2019

This needs a note in updating, otherwise someone might try to install apache-airflow[s3] and be confused by it not working.

That's right. I will add it.

It might be nice to maintain the old names in the extra section for a release or two to ease upgrade too?

It is not necessary to enter a transitional period. Old installations will have already installed libraries. On new installations, the command will be made according to the documentation. It is also worth noting that the transition period is introduced in systems to ensure compatibility of computer systems, not as a time to learn for developer.

I thought to enter a transition period, but by displaying an error message during installation, but I did not find a way. If there was a message for the developer, then a transitional period would make sense. Now we would only move the period where the programmer would be surprised.

@mik-laj
Copy link
Member Author

mik-laj commented Jan 17, 2019

@ashb I added note in UPDATING.md.

@mik-laj mik-laj force-pushed the merge-cloud-providers branch 3 times, most recently from 2832d48 to eebea6b Compare January 21, 2019 15:55
UPDATING.md Outdated
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
the cloud provider.
each cloud provider.

This line break here also doesn't show up when rendered (check out https://github.com/apache/airflow/blob/eebea6bf2f8a70596697d3e976cf18bc97d5cbff/UPDATING.md for example) so this is a bit hard to read.

UPDATING.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

apache-airflow, not airflow :)

@ashb ashb mentioned this pull request Jan 27, 2019
6 tasks
@mik-laj mik-laj force-pushed the merge-cloud-providers branch from eebea6b to d9ea2d7 Compare January 27, 2019 12:04
@mik-laj
Copy link
Member Author

mik-laj commented Jan 27, 2019

@ashb I have introduced your suggestions. Can you look again?

@mik-laj mik-laj force-pushed the merge-cloud-providers branch from d9ea2d7 to dee34c8 Compare February 7, 2019 22:53
@mik-laj
Copy link
Member Author

mik-laj commented Feb 7, 2019

@ashb Rebased and generated preview. http://toothsome-plant.surge.sh/installation.html

Do I still have to do something to get the PRs merged? I received your acceptance, but the change was not merged. I do not understand what it means.

I know you are overwhelmed with work. If I could improve your work in some way, I'm open to suggestion.

@mik-laj mik-laj force-pushed the merge-cloud-providers branch from dee34c8 to 8e7bcab Compare February 7, 2019 23:15
@ashb
Copy link
Member

ashb commented Feb 8, 2019

Review approval says "yes I'm happy with this change" -- the reason it hasn't merged yet is the tests aren't passing, and I hand't dug in to why (transient k8s failure again :( )

wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants