Skip to content

Set MinIO credentials correctly from config before checking them#177

Merged
AgatheZ merged 2 commits into
mainfrom
fix-minio-credentials
Jun 18, 2026
Merged

Set MinIO credentials correctly from config before checking them#177
AgatheZ merged 2 commits into
mainfrom
fix-minio-credentials

Conversation

@Alexiszcv

@Alexiszcv Alexiszcv commented Jun 30, 2025

Copy link
Copy Markdown
Contributor

Closes #176

@Alexiszcv Alexiszcv assigned Alexiszcv and unassigned Alexiszcv Jun 30, 2025
@Alexiszcv Alexiszcv requested a review from AgatheZ June 30, 2025 13:15
AgatheZ
AgatheZ requested changes Jul 1, 2025
View reviewed changes

@AgatheZ AgatheZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As discussed, credentials are fetched from the .env file. However thanks for updating the check_minio_credentials that was buggy

Comment thread mlops/Experiment.py Outdated
os.environ['MLFLOW_TRACKING_URI'] = self.config['server']['MLFLOW_TRACKING_URI']
os.environ['MLFLOW_S3_ENDPOINT_URL'] = self.config['server']['MLFLOW_S3_ENDPOINT_URL']

os.environ['AWS_ACCESS_KEY_ID'] = self.config['minio']['AWS_ACCESS_KEY_ID']

@AgatheZ AgatheZ Jul 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not needed, credentials are fetched from the .env file, not the config file

Comment thread mlops/Experiment.py Outdated
os.environ['MLFLOW_S3_ENDPOINT_URL'] = self.config['server']['MLFLOW_S3_ENDPOINT_URL']

os.environ['AWS_ACCESS_KEY_ID'] = self.config['minio']['AWS_ACCESS_KEY_ID']
os.environ['AWS_SECRET_ACCESS_KEY'] = self.config['minio']['AWS_SECRET_ACCESS_KEY']

@AgatheZ AgatheZ Jul 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here

Comment thread mlops/Experiment.py
def check_minio_credentials(self):
self.auth = boto3.session.Session().get_credentials()
if self.auth is None:
logger.debug('Minio credentials NOT found')

@AgatheZ AgatheZ Jul 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great addition

@mikaelsimard5 mikaelsimard5 force-pushed the fix-minio-credentials branch from c186fb1 to 052cace Compare June 5, 2026 12:28
@github-actions

github-actions Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Coverage

Coverage Report
FileStmtsMissCoverMissing
mlops
   Experiment.py1764177%23, 53–58, 82, 87–88, 110, 112, 117, 173–184, 207–221, 240, 242–243, 292–293, 297–301, 305–306, 315–316
   ProjectFile.py23196%41
   cli.py551769%19–22, 36, 59–66, 86–88, 96–99, 107–108, 113
mlops/release
   Release.py33779%24, 28, 33–38
mlops/release/destinations
   ReleaseDestination.py9189%18
   SharepointDestination.py6267%12–13
   ZenodoDestination.py331942%21–25, 29–35, 43–60
mlops/release/sources
   MLFlowSource.py13654%13–14, 21–27
   ReleaseSource.py14286%14, 23
mlops/utils
   Config.py17170%1–24
TOTAL41611373% 

Tests Skipped Failures Errors Time
17 0 💤 0 ❌ 0 🔥 7m 21s ⏱️

@mikaelsimard5

mikaelsimard5 commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Hey @AgatheZ - just clearing out some old PRs. This one seems mostly useful fixing incorrect logic in check_minio_credentials().

Since this PR is quite old I've rebased on main and resolved conflicts.

Seems like the only thing that was missing to accept this PR was to remove sourcing AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY from the config file as it's already done from the env - I've done this, so should be straightforward to review and close this PR.

@mikaelsimard5 mikaelsimard5 requested a review from AgatheZ June 5, 2026 12:39
AgatheZ
AgatheZ requested changes Jun 11, 2026
View reviewed changes

@AgatheZ AgatheZ left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @mikaelsimard5

Can you please at minimum

  • update mlflow_server/default.env to placeholders

longer term:

  • rotate the prod credentials

@mikaelsimard5 mikaelsimard5 requested a review from AgatheZ June 17, 2026 10:34
@mikaelsimard5

mikaelsimard5 commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Agreed, I've changed to placeholders in default.env and also in the readme that was using a similar nomenclature.

Will rotate prod credentials soon.

@AgatheZ AgatheZ self-assigned this Jun 18, 2026
AgatheZ
AgatheZ approved these changes Jun 18, 2026
View reviewed changes

@AgatheZ AgatheZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me now, thanks

@AgatheZ AgatheZ merged commit 1ac71eb into main Jun 18, 2026
2 checks passed
@AgatheZ AgatheZ deleted the fix-minio-credentials branch June 18, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AgatheZ AgatheZ AgatheZ approved these changes

Assignees

@AgatheZ AgatheZ

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multiple issues with MinIO credentials initialization in Experiment.py

3 participants

@Alexiszcv @mikaelsimard5 @AgatheZ