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-5152]Change autodetect default value to be True #5764

Closed
wants to merge 3 commits into from
Closed

[AIRFLOW-5152]Change autodetect default value to be True #5764

wants to merge 3 commits into from

Conversation

ewljmgzbyydh123
Copy link
Contributor

@ewljmgzbyydh123 ewljmgzbyydh123 commented Aug 8, 2019

Make sure you have checked all steps below.

Jira

Description

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

Set autodetect default value from false to be true to avoid breaking downstream services using GoogleCloudStorageToBigQueryOperator but not aware of the newly added autodetect field. This PR is to fix the current regression introduced by #3880

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

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

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

This seems to be reasonable change but is it backward compatible? If user passes a schema definition and autodetect is set to True will the schema be use instead of autodetection?

Also, please adjust commit and PR message to reference the JIRA issue ✅

@mik-laj mik-laj added the provider:google Google (including GCP) related issues label Aug 9, 2019
@criccomini
Copy link
Contributor

I would argue that the change, itself, was backwards incompatible, and this is restoring the compatibility. We had DAGs that were running fine. After the patch referenced in the JIRA, they broke. This restores proper behavior.

@turbaszek
Copy link
Member

@criccomini thank you for the insight!

@ewljmgzbyydh123
Copy link
Contributor Author

ewljmgzbyydh123 commented Aug 9, 2019

This seems to be reasonable change but is it backward compatible? If user passes a schema definition and autodetect is set to True will the schema be use instead of autodetection?

That's a good catch. Not sure how this autodetect config will ultimately be handled by gcloud, but I think for this case, schema should still be used instead of autodetection. Please refer to codes below:

Also, please adjust commit and PR message to reference the JIRA issue ✅

@ewljmgzbyydh123 ewljmgzbyydh123 changed the title Change default value of autodetect to be True. [AIRFLOW-5152]Change default value of autodetect to be True. Aug 9, 2019
@ewljmgzbyydh123 ewljmgzbyydh123 changed the title [AIRFLOW-5152]Change default value of autodetect to be True. [AIRFLOW-5152]Change autodetect default value to be True Aug 9, 2019
@ewljmgzbyydh123
Copy link
Contributor Author

Declining this PR and created #5771 since I messed up the commit messages.

@turbaszek
Copy link
Member

In such cases git commit --amend may help, it's quite useful:
https://help.github.com/en/articles/changing-a-commit-message

@ashb
Copy link
Member

ashb commented Aug 13, 2019

@criccomini Should we pull this (well the re-opened one) in as a bugfix for 1.10.5 then?

@criccomini
Copy link
Contributor

@ashb that would be ideal, if possible. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants