chore(ci): unquarantine media download test #1039
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Disclaimer: Experimental PR review
PR Summary
This PR modifies the CI workflow to fix Docker networking issues by changing the S3 event upload endpoint binding, enabling previously quarantined media download tests to run successfully.
- Changed S3 event upload endpoint from
localhost:9090to0.0.0.0:9090in/.github/workflows/ci.ymlto allow container-to-container communication - Unquarantined
test_replace_media_reference_string_in_object()in/tests/test_media.pyby removing skip decorator - Removed unused
tmp_pathparameter from media reference test - Test now validates WAV audio file handling including reference creation, storage and retrieval
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
This PR updates the CI configuration by changing the S3 media upload endpoint configuration from 'LANGFUSE_S3_EVENT_UPLOAD_ENDPOINT' to 'LANGFUSE_S3_MEDIA_UPLOAD_ENDPOINT' with endpoint URL 'http://localhost:9090'.
- Changed environment variable name from
LANGFUSE_S3_EVENT_UPLOAD_ENDPOINTtoLANGFUSE_S3_MEDIA_UPLOAD_ENDPOINTin/.github/workflows/ci.yml - Updated endpoint URL back to
http://localhost:9090fromhttp://0.0.0.0:9090for container networking - Maintained test unquarantine for media reference string handling
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
No significant changes found since the last review. The current state shows a discrepancy between the PR description (which mentions changing to http://0.0.0.0:9090) and the actual implementation (which uses http://localhost:9090), but this was already noted in the previous review.
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -90,7 +90,7 @@ jobs: | |||
| rm -rf .env | |||
|
|
|||
| echo "::group::Run server" | |||
| TELEMETRY_ENABLED=false LANGFUSE_S3_EVENT_UPLOAD_ENDPOINT=http://localhost:9090 LANGFUSE_SDK_CI_SYNC_PROCESSING_ENABLED=true LANGFUSE_READ_FROM_POSTGRES_ONLY=true LANGFUSE_READ_FROM_CLICKHOUSE_ONLY=false LANGFUSE_RETURN_FROM_CLICKHOUSE=false docker compose up -d | |||
| TELEMETRY_ENABLED=false LANGFUSE_S3_MEDIA_UPLOAD_ENDPOINT=http://localhost:9090 LANGFUSE_SDK_CI_SYNC_PROCESSING_ENABLED=true LANGFUSE_READ_FROM_POSTGRES_ONLY=true LANGFUSE_READ_FROM_CLICKHOUSE_ONLY=false LANGFUSE_RETURN_FROM_CLICKHOUSE=false docker compose up -d | |||
There was a problem hiding this comment.
logic: PR description mentions changing endpoint to http://0.0.0.0:9090 but it's still set to http://localhost:9090. This may cause Docker networking issues.
Important
Unquarantine
test_replace_media_reference_string_in_object()and update Docker networking in CI configuration.test_replace_media_reference_string_in_object()intests/test_media.pyby removing@pytest.mark.skip.LANGFUSE_S3_EVENT_UPLOAD_ENDPOINTfromhttp://localhost:9090tohttp://0.0.0.0:9090in.github/workflows/ci.ymlto address Docker networking issues.This description was created by
for f57e5aa. It will automatically update as commits are pushed.