Skip to content

Conversation

cveticm
Copy link
Collaborator

@cveticm cveticm commented Sep 30, 2025

Proposed Changes

We shouldn't expose the database username and password to an LLM by passing it along as inputs.

  • Removes dbUsername and dbPassword as inputs for get_connection_string
    • Replaces GetConnectionStringOptions with just passing the remaining input container_id_or_name directly
  • Gets dbusername and dbpassword within get_connnection_string by reusing logic in get_deploymnet_id
  • Adjusts testing

@coveralls
Copy link
Collaborator

coveralls commented Sep 30, 2025

Pull Request Test Coverage Report for Build 18165422202

Details

  • 66 of 67 (98.51%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.816%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/client/get_mongodb_secret.rs 21 22 95.45%
Totals Coverage Status
Change from base Build 18001852096: 0.0%
Covered Lines: 3247
Relevant Lines: 3253

💛 - Coveralls

@cveticm cveticm marked this pull request as ready for review September 30, 2025 12:41
if let Some(env_value) = value(deployment) {
return Ok(Some(env_value.to_string()));
}
pub async fn get_mongodb_secret<D: RunCommandInContainer>(
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this function to a separate file?
It doesn't necessarily belong in get_connection_string.rs or get_deployment_id.rs.
Maybe it would make more sense to move this to a new get_mongodb_secret file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, moved 👍

@cveticm cveticm enabled auto-merge (squash) October 1, 2025 14:35
@cveticm cveticm disabled auto-merge October 1, 2025 14:40
@cveticm cveticm merged commit 532d5f3 into main Oct 1, 2025
15 of 17 checks passed
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.

4 participants