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

feat(ingest): Add option to change name of database for postgres #2898

Conversation

aseembansal-gogo
Copy link
Contributor

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

@aseembansal-gogo aseembansal-gogo force-pushed the add-database-identifier-postgres branch from ce1e6e0 to 300db20 Compare July 16, 2021 11:17
Copy link
Collaborator

@hsheth2 hsheth2 left a comment

Choose a reason for hiding this comment

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

LGTM

In the future, we'll probably want a more principled/general way of handling this sort of modification, and ideally would have transformers to handle this. As such, I'm thinking about these changes are more of a stopgap measure.

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

Suggest we name the field as database_alias instead of database_identifier

metadata-ingestion/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM!

@aseembansal-gogo
Copy link
Contributor Author

@shirshanka I am not sure where to add the missing dependency for the test. Can you please suggest which file needs this change?

@shirshanka
Copy link
Contributor

shirshanka commented Jul 18, 2021

@aseembansal-gogo : this would be in setup.py

Add postgres to the base_dev_requirements variable.
near:

      *list(
         dependency
            for plugin in [
                    "bigquery",
                    "bigquery-usage",
                    "looker",
                    "glue",
                    "oracle",
                    "sagemaker",

@shirshanka shirshanka merged commit 6e1b2cf into datahub-project:master Jul 20, 2021
@aseembansal-gogo aseembansal-gogo deleted the add-database-identifier-postgres branch July 23, 2021 04:17
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