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

🎉 Make credentials optional in BigQuery connector (issue #3657) #3947

Merged
merged 2 commits into from
Jun 17, 2021
Merged

🎉 Make credentials optional in BigQuery connector (issue #3657) #3947

merged 2 commits into from
Jun 17, 2021

Conversation

sabifranjo
Copy link
Contributor

@sabifranjo sabifranjo commented Jun 8, 2021

What

Issue #3657
image

How

Solved by making the field optional and using default application credentials in code when the field is empty.

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met

  • Unit & integration tests added as appropriate (and are passing)
    image

  • /test command documented here is passing.

    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed

  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.

  • Documentation updated

    • README
    • CHANGELOG.md
    • Reference docs in the docs/integrations/ directory.
  • Build is successful

  • Connector version bumped like described here

  • New Connector version released on Dockerhub by running the /publish command described here

  • No major blockers

  • PR merged into master branch

  • Follow up tickets have been created

  • Associated tickets have been closed & stakeholders notified

Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

@sabifranjo I am concerned that this is a breaking change/backwards incompatible. While I agree that this is the right way to solve this issue, we don't have a way to automatically gracefully upgrade connectors when a breaking change has been made to the config. Can we instead leave the schema the same and just remove credentials_json as a required field? We should also update its description to clarify that not including it would use the "default" authentication.

In the future once we have a graceful migration process for breaking changes on connectors this should be solved more elegantly.

@sabifranjo
Copy link
Contributor Author

@sabifranjo I am concerned that this is a breaking change/backwards incompatible. While I agree that this is the right way to solve this issue, we don't have a way to automatically gracefully upgrade connectors when a breaking change has been made to the config. Can we instead leave the schema the same and just remove credentials_json as a required field? We should also update its description to clarify that not including it would use the "default" authentication.

In the future once we have a graceful migration process for breaking changes on connectors this should be solved more elegantly.

@sherifnada I did the changes and updated the description accordingly. So now only the destination-bigquery:0.3.5 image needs to be published.

@github-actions github-actions bot added the area/connectors Connector related issues label Jun 15, 2021
@marcosmarxm
Copy link
Member

@sabifranjo sorry took so long! I'm going to review this max later today! thanks for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants