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-5643] Reduce duplicated logic in S3Hook #6313

Merged
merged 1 commit into from
Oct 16, 2019
Merged

[AIRFLOW-5643] Reduce duplicated logic in S3Hook #6313

merged 1 commit into from
Oct 16, 2019

Conversation

louisguitton
Copy link
Contributor

@louisguitton louisguitton commented Oct 12, 2019 •
edited
Loading

Jira

Description

  • load_bytes is a convenience method that delegates to load_fileobj, just like load_string delegates to load_bytes. This change is here to make this logic more apparent.

Tests

  • No new functionality introduced
./breeze --backend postgres --test-target tests.hooks.test_s3_hook

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

  • No new functionality introduced

@louisguitton louisguitton changed the title [NO-TICKET] Clarify logic S3Hook [NO-TICKET] Reduce duplicated logic around S3Hook Oct 12, 2019
@louisguitton louisguitton changed the title [NO-TICKET] Reduce duplicated logic around S3Hook [NO-TICKET] Reduce duplicated logic in S3Hook Oct 12, 2019
@louisguitton louisguitton changed the title [NO-TICKET] Reduce duplicated logic in S3Hook [AIRFLOW-5643] Reduce duplicated logic in S3Hook Oct 12, 2019
@louisguitton louisguitton marked this pull request as ready for review October 12, 2019 09:54
@codecov-io
Copy link

codecov-io commented Oct 12, 2019 •
edited
Loading

Codecov Report

Merging #6313 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6313      +/-   ##
==========================================
+ Coverage   80.34%   80.35%   +<.01%     
==========================================
  Files         616      616              
  Lines       35733    35724       -9     
==========================================
- Hits        28711    28706       -5     
+ Misses       7022     7018       -4
Impacted Files Coverage Δ
airflow/hooks/S3_hook.py 95.62% <100%> (+1.35%) ⬆️
airflow/utils/dag_processing.py 56.55% <0%> (-0.35%) ⬇️
airflow/models/taskinstance.py 93.77% <0%> (+0.5%) ⬆️

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 c0d98a7...1fc26b2. Read the comment docs.

@mik-laj mik-laj added the provider:amazon-aws AWS/Amazon - related issues label Oct 13, 2019
feluelle
feluelle reviewed Oct 14, 2019
View reviewed changes
Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

Just my opinion: I rather create a private function that will be called in public functions. So that you do not rely on another public function (in case the public function gets changed).

@louisguitton
Copy link
Contributor Author

Thank you both for your reviews. I agree with you @feluelle , I'll make the suggested changes

@louisguitton
[AIRFLOW-5643] Reduce duplicated logic in S3Hook
c841579 
S3Hook.load_bytes is duplicating the logic of S3Hook.load_file_obj.
Instead, we should stay consistent : S3Hook.load_string is already
delegating the logic to S3Hook.load_bytes, so we can use the same
approach to delegate to S3Hook.load_file_obj
feluelle
feluelle approved these changes Oct 16, 2019
View reviewed changes
Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

Nice!

@feluelle
Copy link
Member

I restarted the failed travis checks.

@potiuk
Copy link
Member

potiuk commented Oct 16, 2019

If those are the kubernetes ones - maybe merge it without waiting. We have found with @gerardo a reason why kubernetes tests might timeout and we will fix it shortly - but I do not want to hold it back!

ashb
ashb requested changes Oct 16, 2019
View reviewed changes
airflow/hooks/S3_hook.py Show resolved Hide resolved
@louisguitton
Copy link
Contributor Author

If those are the kubernetes ones - maybe merge it without waiting. We have found with @gerardo a reason why kubernetes tests might timeout and we will fix it shortly - but I do not want to hold it back!

Yeah the 2 failing tests are the k8s ones :/

@feluelle
Copy link
Member

Yes, Jarek I just saw the thread right after I reran the tests.

I think we can merge it then. 👍

@feluelle feluelle merged commit ac42428 into apache:master Oct 16, 2019
@louisguitton louisguitton deleted the s3-hook branch October 16, 2019 17:18
ashb pushed a commit that referenced this pull request Dec 14, 2019
@louisguitton @ashb
[AIRFLOW-5643] Reduce duplicated logic in S3Hook (#6313)
f0e00dd 
S3Hook.load_bytes is duplicating the logic of S3Hook.load_file_obj.
Instead, we should stay consistent : S3Hook.load_string is already
delegating the logic to S3Hook.load_bytes, so we can use the same
approach to delegate to S3Hook.load_file_obj

(cherry picked from commit ac42428)
ashb pushed a commit that referenced this pull request Dec 16, 2019
@louisguitton @ashb
[AIRFLOW-5643] Reduce duplicated logic in S3Hook (#6313)
38c8f3d 
S3Hook.load_bytes is duplicating the logic of S3Hook.load_file_obj.
Instead, we should stay consistent : S3Hook.load_string is already
delegating the logic to S3Hook.load_bytes, so we can use the same
approach to delegate to S3Hook.load_file_obj

(cherry picked from commit ac42428)
kaxil pushed a commit that referenced this pull request Dec 17, 2019
@louisguitton @kaxil
[AIRFLOW-5643] Reduce duplicated logic in S3Hook (#6313)
c4e4fbd 
S3Hook.load_bytes is duplicating the logic of S3Hook.load_file_obj.
Instead, we should stay consistent : S3Hook.load_string is already
delegating the logic to S3Hook.load_bytes, so we can use the same
approach to delegate to S3Hook.load_file_obj

(cherry picked from commit ac42428)
ashb pushed a commit that referenced this pull request Dec 19, 2019
@louisguitton @ashb
[AIRFLOW-5643] Reduce duplicated logic in S3Hook (#6313)
43fdea0 
S3Hook.load_bytes is duplicating the logic of S3Hook.load_file_obj.
Instead, we should stay consistent : S3Hook.load_string is already
delegating the logic to S3Hook.load_bytes, so we can use the same
approach to delegate to S3Hook.load_file_obj

(cherry picked from commit ac42428)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashb ashb ashb approved these changes

@milton0825 milton0825 milton0825 approved these changes

@feluelle feluelle feluelle approved these changes

Assignees
No one assigned
Labels
provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@louisguitton @codecov-io @feluelle @potiuk @ashb @milton0825 @mik-laj