Skip to content

MIDRC-1292 Fix S3 proxy for Nextflow workflows against Minio - #169

Merged
paulineribeyre merged 4 commits into
masterfrom
fix-minio-nextflow
Jul 23, 2026
Merged

MIDRC-1292 Fix S3 proxy for Nextflow workflows against Minio#169
paulineribeyre merged 4 commits into
masterfrom
fix-minio-nextflow

Conversation

@paulineribeyre

@paulineribeyre paulineribeyre commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/MIDRC-1292

New Features

Breaking Changes

Bug Fixes

  • Fix S3 proxy for Nextflow workflows against Minio

Improvements

Dependency updates

Deployment changes

@github-actions

Copy link
Copy Markdown

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@github-actions

Copy link
Copy Markdown

Integration Tests on Kind

filepath passed skipped SUBTOTAL
tests/test_gen3_workflow.py 27 2 29
TOTAL 27 2 29

@github-actions

Copy link
Copy Markdown

Integration Tests

Test summary after running integration tests

filepath passed failed error skipped SUBTOTAL
tests/test_gen3_workflow.py 9 1 18 1 29
TOTAL 9 1 18 1 29

Test summary after rerunning failed integration tests

filepath passed failed SUBTOTAL
tests/test_gen3_workflow.py 18 1 19
TOTAL 18 1 19

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

@github-actions

Copy link
Copy Markdown

Integration Tests

filepath passed SUBTOTAL
tests/test_gen3_workflow.py 2 2
TOTAL 2 2

Comment thread gen3workflow/routes/s3.py Outdated
# which represents a string-string map". Band-aid fix: drop it
# See https://github.com/minio/minio-go/issues/2235
headers.pop("x-amz-server-side-encryption-context", None)
# TODO: this should be fixed in the latest version of Funnel... test it

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.

The funnel PR with this change is not merged yet. See -- calypr/funnel#1428

We could test it first in a kind setup and then merge it. For now, we could just update the TODO comment to reflect the same.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah i guess that's what i meant here - i don't want to priorize testing it now, but i added the note so we remember to do it later

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i can update the comment - waiting to see if you have any other comments

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

Left some questions/comments. Will approve once answered/changes made

uses: uc-cdis/.github/.github/workflows/integration_tests.yaml@master
with:
SERVICE_TO_TEST: gen3_workflow
TEST_REPO_BRANCH: enable-kind-nextflow

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.

Are you planning to merge the TEST REPO BRANCH along with this PR?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I would merge this PR first. There's another change in the gen3-code-vigil branch that i want to test

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.

Gotcha. We need to make sure this is removed before we delete the code-vigil branch.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh i'm just removing it before merging! There are no breaking changes here

SETUP_SCRIPT_1: https://raw.githubusercontent.com/uc-cdis/gen3-workflow/refs/heads/${{ github.event.pull_request.head.ref }}/.github/workflows/integration_tests_on_kind/ci_start_kind_cluster.sh
SETUP_SCRIPT_2: https://raw.githubusercontent.com/uc-cdis/gen3-workflow/refs/heads/${{ github.event.pull_request.head.ref }}/.github/workflows/integration_tests_on_kind/ci_override_config_and_start_minio.sh
SETUP_SCRIPT_3: https://raw.githubusercontent.com/uc-cdis/gen3-workflow/refs/heads/${{ github.event.pull_request.head.ref }}/.github/workflows/integration_tests_on_kind/ci_expose_kind_cluster.sh
TEST_REPO_BRANCH: enable-kind-nextflow

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 as above.

nss10
nss10 previously approved these changes Jul 23, 2026

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

LGTM.

@paulineribeyre
paulineribeyre merged commit 8546fde into master Jul 23, 2026
10 of 12 checks passed
@paulineribeyre
paulineribeyre deleted the fix-minio-nextflow branch July 23, 2026 17:37
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.

2 participants