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

Initial Spec For Credential Management and SQLAlchemy Database Connectors #1420

Open
wants to merge 27 commits into
base: main
Choose a base branch
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
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 ?

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 ?

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 ?

@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.



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') ?

Add Integration Test
Copy link

github-actions bot commented Jan 28, 2025

✅ 2/2 passed, 2s total

Running from acceptance #64

@sundarshankar89 sundarshankar89 added this to the Profiler milestone Feb 3, 2025
@sundarshankar89 sundarshankar89 changed the base branch from main to feature/simplified_installation February 5, 2025 04:40
@sundarshankar89 sundarshankar89 added the stacked PR Should be reviewed, but not merged label Feb 5, 2025
Base automatically changed from feature/simplified_installation to main February 5, 2025 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/profiler stacked PR Should be reviewed, but not merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants