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.

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

Initial Spec For Credential Management and SQLAlchemy Database Connectors #1420

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Initial Spec For Credential Management and SQLAlchemy Database Connectors #1420

wants to merge 24 commits into from

Conversation

sundarshankar89
Copy link
Collaborator

Initial Spec For Credential Management and SQLAlchemy Database Connectors

@sundarshankar89 sundarshankar89 self-assigned this Jan 15, 2025
@sundarshankar89 sundarshankar89 marked this pull request as ready for review January 21, 2025 12:56
@sundarshankar89 sundarshankar89 requested a review from a team as a code owner January 21, 2025 12:56
ericvergnaud
ericvergnaud requested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@ericvergnaud ericvergnaud left a comment

Choose a reason for hiding this comment

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

Can we spin up docker instances with pre-installed DB servers and write integration tests such that we can prove that at least some of this works ?

src/databricks/labs/remorph/connections/credential_manager.py Outdated
with open(file_path, encoding="utf-8") as f:
return yaml.safe_load(f)

def get(self, source: str) -> dict[str, str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

get what ?

src/databricks/labs/remorph/connections/credential_manager.py Outdated
raise KeyError(error_msg)
raise KeyError(error_msg)

def get_secret_value(self, key: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't get_secret suffice ?

src/databricks/labs/remorph/connections/database_manager.py Show resolved Hide resolved
@sundarshankar89
Copy link
Collaborator Author

Can we spin up docker instances with pre-installed DB servers and write integration tests such that we can prove that at least some of this works?

There is infra to test it I will add an integration test using envgetter.

ericvergnaud
ericvergnaud requested changes Jan 24, 2025
View reviewed changes
src/databricks/labs/remorph/connections/env_getter.py


class EnvGetter:
def __init__(self, is_debug: bool = False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not read debug-env.json when the app starts and set env variables accordingly (using os.environ['my_name'] = 'my_value') ?

@sundarshankar89
add acceptance (#1428)
79e3a86 
Add Integration Test
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 28, 2025 •
edited
Loading

✅ 2/2 passed, 0s total

Running from acceptance #53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sriram251-code sriram251-code sriram251-code left review comments

@ericvergnaud ericvergnaud ericvergnaud requested changes

@bishwajit-db bishwajit-db Awaiting requested review from bishwajit-db

Requested changes must be addressed to merge this pull request.

Assignees

@sundarshankar89 sundarshankar89

Labels
feature/profiler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sundarshankar89 @ericvergnaud @sriram251-code