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

Integration test for run_dicom_archive_loader.py #1203

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

Conversation

maximemulder
Copy link
Contributor

@maximemulder maximemulder commented Oct 18, 2024

This PR mounts the S3 bucket in CI and adds an integration test for run_dicom_archive_loader.py !

@maximemulder maximemulder added the A-CI Area: CI. Issues and pull requests related to CI, including automated tests and static checks label Oct 18, 2024
@maximemulder maximemulder changed the title Use Raisinbread tests in integration tests Use Raisinbread files in integration tests Oct 18, 2024
@maximemulder maximemulder changed the title Use Raisinbread files in integration tests Use imaging files in integration tests Oct 18, 2024
@maximemulder maximemulder force-pushed the 2024-10-18_integration-s3 branch 25 times, most recently from 63dc912 to 25bd52d Compare October 18, 2024 22:15
@maximemulder maximemulder force-pushed the 2024-10-18_integration-s3 branch 8 times, most recently from f0c2278 to dd3886c Compare October 31, 2024 19:49
@maximemulder maximemulder changed the title Use imaging files in integration tests Integration test for run_dicom_archive_loader.py Oct 31, 2024
@maximemulder
Copy link
Contributor Author

Still kind of barebones but better than nothing for now IMO.

@maximemulder maximemulder marked this pull request as ready for review November 6, 2024 21:07
Comment on lines 16 to 17
BUCKET_ACCESS_KEY: lorisadmin-ro
BUCKET_SECRET_KEY: Tn=qP3LupmXnMuc
Copy link
Collaborator

Choose a reason for hiding this comment

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

@maximemulder could this be secrets variables? I know this is for read-only data but still, not ideal to get credentials in the code.

Copy link
Contributor Author

@maximemulder maximemulder Nov 19, 2024

Choose a reason for hiding this comment

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

For secrets:
https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions#using-encrypted-secrets-in-a-workflow

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

For variables:
https://github.com/orgs/community/discussions/44322

Variables are not passed to workflows that are triggered by a pull request from a fork.

It seems we cannot pass variable or secrets in pull requests triggered from a fork (i.e., all our PRs). I guess our choices are either:

  • Make the variables secret, but the integration tests will need to be triggered manually.
  • Hard-code the variables in the code (which makes them public), and the tests can be triggered automatically.

@maximemulder maximemulder force-pushed the 2024-10-18_integration-s3 branch 2 times, most recently from 4535a88 to ac4dcf7 Compare November 19, 2024 16:52
@maximemulder maximemulder force-pushed the 2024-10-18_integration-s3 branch 7 times, most recently from 2ab4bd1 to 0444bdb Compare November 19, 2024 19:47
@maximemulder
Copy link
Contributor Author

There is now a test to check that a file has indeed been inserted in the database !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: CI. Issues and pull requests related to CI, including automated tests and static checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants