Skip to content

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

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
Closed
ewljmgzbyydh123 wants to merge 3 commits into from
Closed

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

ewljmgzbyydh123 wants to merge 3 commits into from

Conversation

@ewljmgzbyydh123
Copy link
Contributor

@ewljmgzbyydh123 ewljmgzbyydh123 commented Aug 8, 2019 •
edited
Loading

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

turbaszek
turbaszek reviewed Aug 9, 2019
View reviewed changes
Copy link
Member

@turbaszek turbaszek left a comment •
edited
Loading

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

criccomini commented Aug 9, 2019

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

turbaszek commented Aug 9, 2019

@criccomini thank you for the insight!

@ewljmgzbyydh123
Copy link
Contributor Author

ewljmgzbyydh123 commented Aug 9, 2019 •
edited
Loading

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 ewljmgzbyydh123 mentioned this pull request Aug 10, 2019
Merged
6 tasks
@ewljmgzbyydh123
Copy link
Contributor Author

ewljmgzbyydh123 commented Aug 10, 2019

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

@turbaszek
Copy link
Member

turbaszek commented Aug 10, 2019

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

criccomini commented Aug 16, 2019

@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

Reviewers

@turbaszek turbaszek turbaszek left review comments

Assignees

No one assigned

Labels

provider:google Google (including GCP) related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ewljmgzbyydh123 @criccomini @turbaszek @ashb @mik-laj