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-2563] Fix PigCliHook Python 3 string/bytes use #3594

Closed
wants to merge 1 commit into from
Closed

[AIRFLOW-2563] Fix PigCliHook Python 3 string/bytes use #3594

wants to merge 1 commit into from

Conversation

jakahn
Copy link
Contributor

@jakahn jakahn commented Jul 11, 2018

Make sure you have checked all steps below.

JIRA

Description

  • This PR fixes PigCliHook's Python 2-style use of strings where Python 3 requires (or returns) a bytes-like object. It also introduces unit tests for PigCliHook, which were missing.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    • tests/hooks/test_pig_hook.py

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
    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 non-test files or new functionality.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@codecov-io
Copy link

codecov-io commented Jul 11, 2018

Codecov Report

Merging #3594 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3594      +/-   ##
==========================================
+ Coverage   76.85%   77.12%   +0.26%     
==========================================
  Files         204      204              
  Lines       15522    15522              
==========================================
+ Hits        11930    11971      +41     
+ Misses       3592     3551      -41
Impacted Files Coverage Δ
airflow/hooks/pig_hook.py 100% <100%> (+100%) ⬆️

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 aa17b5b...f759808. Read the comment docs.

Unit tests added for PigCliHook as well to prevent future issues.
@jakahn jakahn closed this Jul 12, 2018
@jakahn jakahn reopened this Jul 12, 2018
@jakahn
Copy link
Contributor Author

jakahn commented Jul 12, 2018

The failing test appears to have been a flake (Travis reports a 10 minute period of no output on an unrelated test; all tests pass on my fork).

@jakahn jakahn closed this Jul 12, 2018
@jakahn jakahn reopened this Jul 12, 2018
@Fokko
Copy link
Contributor

Fokko commented Jul 13, 2018

Please link to your fork instead of retriggering every time.

@jakahn
Copy link
Contributor Author

jakahn commented Jul 13, 2018

@jakahn
Copy link
Contributor Author

jakahn commented Jul 27, 2018

Just bumping since it's been ~2 weeks

@artwr
Copy link
Contributor

artwr commented Jul 27, 2018

LGTM +1

@asfgit asfgit closed this in 3e7e42f Jul 27, 2018
lxneng pushed a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018
Unit tests added for PigCliHook as well to prevent
future issues.

Closes apache#3594 from jakahn/master
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
Unit tests added for PigCliHook as well to prevent
future issues.

Closes apache#3594 from jakahn/master
ashb pushed a commit that referenced this pull request Apr 6, 2019
Unit tests added for PigCliHook as well to prevent
future issues.

Closes #3594 from jakahn/master
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.

4 participants