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

(chore) Supporting dbt sync without updating the db connection configuration on Superset #193

Merged
merged 5 commits into from
Mar 16, 2023

Conversation

Vitor-Avila
Copy link
Contributor

Currently, the CLI would always update the DB connection configuration on the Workspace using the information from the profiles.yml file (regardless if --import-db was passed or not). The flag was only validated in case no database connection was found.

This PR introduces the ability to perform a dbt sync without updating the DB connection. This is very useful in case the dbt configuration uses different credentials, or even a different authentication method that might not be supported by Superset.

@@ -78,6 +80,16 @@ def sync_database( # pylint: disable=too-many-locals, too-many-arguments
**meta,
)

database["sqlalchemy_uri"] = connection_params["sqlalchemy_uri"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, this would always be executed (which pretty much builds the SQLAlchemy URI from the connection settings on the profiles.yml). Now it's only executed in case --import-db is passed.

elif databases:
_logger.info("Found an existing database connection, using it")
database = databases[0]
database["sqlalchemy_uri"] = client.get_database(database["id"])[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case --import-db is not passed, I added this to fetch the SQLAlchemy URI from the Superset API instead. The password wouldn't be included, but I don't think this would be a problem since we're not updating the DB connection.

@dweaver33
Copy link

I've covered this fix here: #169

@Vitor-Avila
Copy link
Contributor Author

Hey @dweaver33,

Thank you so much for your interest on contributing to the CLI. I'm afraid that due to license restrictions, we're currently not accepting contributions from outside of Preset. I was actually going to add a comment to your PR/Issue once this gets merged.

Thank you so much for your understanding.

@dweaver33
Copy link

Hey @dweaver33,

Thank you so much for your interest on contributing to the CLI. I'm afraid that due to license restrictions, we're currently not accepting contributions from outside of Preset. I was actually going to add a comment to your PR/Issue once this gets merged.

Thank you so much for your understanding.

Ha! Didn't realize you were a maintainer here.

In that case, can you also look at my fix in that same PR for the exposures? #177

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks, Vitor!

@betodealmeida betodealmeida merged commit 8a3a8b3 into preset-io:main Mar 16, 2023
@Vitor-Avila
Copy link
Contributor Author

Hey @dweaver33,
Thank you so much for your interest on contributing to the CLI. I'm afraid that due to license restrictions, we're currently not accepting contributions from outside of Preset. I was actually going to add a comment to your PR/Issue once this gets merged.
Thank you so much for your understanding.

Ha! Didn't realize you were a maintainer here.

In that case, can you also look at my fix in that same PR for the exposures? #177

I'll take a look on that ticket, @dweaver33. Thank you so much!

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.

3 participants