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-3396] Make sql param as required in BigQueryOperator #4224

Merged
merged 1 commit into from
Dec 1, 2018

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Nov 22, 2018

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"

Description

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

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

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

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes flake8

@kaxil kaxil requested a review from Fokko November 22, 2018 16:07
@kaxil
Copy link
Member Author

kaxil commented Nov 22, 2018

Instead of allowing None and then adding condition, we should remove it.

This was made earlier when we had bql and sql and wanted to deprecate bql

@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4224      +/-   ##
=========================================
+ Coverage    77.8%   78.9%   +1.09%     
=========================================
  Files         201     201              
  Lines       16360   17675    +1315     
=========================================
+ Hits        12729   13946    +1217     
- Misses       3631    3729      +98
Impacted Files Coverage Δ
airflow/utils/dag_processing.py 56.13% <0%> (-1.4%) ⬇️
airflow/jobs.py 77.36% <0%> (+0.27%) ⬆️
airflow/configuration.py 89.39% <0%> (+0.34%) ⬆️
airflow/models.py 95.14% <0%> (+2.8%) ⬆️

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 9f7f5e4...b819235. Read the comment docs.

@bolkedebruin
Copy link
Contributor

@kaxil please use a jira for this, it doesnt qualify as a doc change ;-)

@kaxil kaxil changed the title [AIRFLOW-XXX] Make sql param as required in BigQueryOperator [AIRFLOW-3396] Make sql param as required in BigQueryOperator Nov 25, 2018
@kaxil
Copy link
Member Author

kaxil commented Nov 25, 2018

@bolkedebruin :) Updated.

@kaxil
Copy link
Member Author

kaxil commented Nov 28, 2018

ping @Fokko

@kaxil kaxil requested a review from ashb December 1, 2018 01:41
@ashb
Copy link
Member

ashb commented Dec 1, 2018

When did we release the change that deprecated/removed BQL?

The code change is fine, just wondering about upgrade paths for people with by back-compat hat on 🎩

@kaxil
Copy link
Member Author

kaxil commented Dec 1, 2018

@ashb We issued deprecation warning in 1.10.0:

https://github.com/apache/incubator-airflow/blob/1.10.0/airflow/contrib/operators/bigquery_operator.py

and we haven't yet removed it in 1.10.1 and don't plan to remove it in 1.10.2 as well.

@kaxil kaxil merged commit b4b1577 into apache:master Dec 1, 2018
@kaxil kaxil deleted the kaxil-patch-3 branch December 1, 2018 21:35
elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
wmorris75 pushed a commit to modmed/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.

4 participants