-
-
Notifications
You must be signed in to change notification settings - Fork 278
fix: Correct PyPI release workflow permissions and tag handling #259
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
Conversation
Problem:
- Naver API 'count' param drifts daily (days since fixed date)
- KRX date params (strtDd, endDd, etc.) cause match failures
- 7 cassettes recorded with stale values → tests failed
Solution:
1. Custom VCR matchers ignore volatile date/count parameters
- uri_without_dates: strips IGNORED_DATE_KEYS from query
- form_body_matcher: order-insensitive, ignores date keys
- before_record_request: scrubs params before saving
2. Regenerated 7 time-sensitive cassettes with current baseline:
- market/stock_ohlcv_by_date_*
- index/portfolio_deposit_file_*
- etf/{ticker,etn_ticker,elw_ticker}_list
- fetch full git history & tags, add version verification
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request aims to fix CD workflow issues for PyPI releases but also includes significant changes to test infrastructure. The PR adds workflow permissions, enables full git history fetching for version detection, and refactors VCR (cassette recording) configuration to better handle volatile date parameters in tests.
Changes:
- Enhanced CD workflow with explicit permissions, full git history/tag fetching, and version verification step
- Refactored VCR test configuration to filter volatile date parameters, preventing unnecessary cassette re-recording
- Regenerated test cassettes with updated timestamps and request parameters
Reviewed changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/cd.yml |
Added workflow permissions, git fetch configuration for tags, and setuptools-scm version verification |
tests/conftest.py |
Introduced date-filtering logic for VCR matchers, custom URI and body matchers, and request scrubbing |
tests/cassettes/etf/ticker_list.yaml |
Regenerated cassette with updated dates and timestamps |
tests/cassettes/etf/etn_ticker_list.yaml |
Regenerated cassette with updated dates and Accept-Encoding headers |
tests/cassettes/etf/elw_ticker_list.yaml |
Regenerated cassette with updated dates and Accept-Encoding headers |
tests/conftest.py
Outdated
|
|
||
| cassette_name = marker.args[0] | ||
| with custom_vcr.use_cassette(cassette_name, **vcr_config): | ||
| with custom_vcr.use_cassette(cassette_name): |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use_cassette fixture accepts a vcr_config parameter (line 116) but doesn't use it. Line 148 uses the global custom_vcr instance directly instead of creating a VCR instance with the provided config. This makes the vcr_config fixture (lines 106-112) and the function parameter unused. Either use the config parameter to create a VCR instance, or remove the unused parameter and fixture if they're not needed.
| with custom_vcr.use_cassette(cassette_name): | |
| vcr_instance = vcr.VCR(**vcr_config) | |
| with vcr_instance.use_cassette(cassette_name): |
| @@ -1,8 +1,58 @@ | |||
| import urllib.parse | |||
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description focuses exclusively on fixing PyPI release workflow issues (permissions, tag handling, credential management), but the changes include significant modifications to test infrastructure (VCR configuration, date filtering, custom matchers) and regenerated test cassettes. These test-related changes should be mentioned in the PR description to provide a complete picture of what this PR modifies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| - name: Verify version | ||
| run: | | ||
| python -m pip install setuptools-scm | ||
| python -m setuptools_scm | ||
|
|
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setuptools-scm package is already declared in pyproject.toml's build-system.requires, so it will be installed automatically when running python -m build in the next step. This separate installation step is redundant. Consider removing this step or moving it after the build step if the intent is to verify the version was correctly detected.
| - name: Verify version | |
| run: | | |
| python -m pip install setuptools-scm | |
| python -m setuptools_scm |
| # Allow either body to be a superset after volatile keys are stripped | ||
| return b1 == b2 or b1.issuperset(b2) or b2.issuperset(b1) |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The superset matching logic (line 80) allows requests to match even when they have different sets of parameters after filtering. For example, if the recorded cassette has parameters {A, B} and the new request has parameters {A, B, C} (or vice versa), they will match. This could cause tests to pass when they should fail, masking real issues where extra or missing parameters are sent to the API. Consider using strict equality (b1 == b2) or document why superset matching is necessary for this use case.
| # Debug logging | ||
| if not result: | ||
| import sys | ||
|
|
||
| print("\n[VCR URI Matcher Debug]", file=sys.stderr) | ||
| print(f" Request URI: {r1.uri}", file=sys.stderr) | ||
| print(f" Cassette URI: {r2.uri}", file=sys.stderr) | ||
| print(f" Normalized Request: {norm1}", file=sys.stderr) | ||
| print(f" Normalized Cassette: {norm2}", file=sys.stderr) | ||
| print(f" Match: {result}\n", file=sys.stderr) |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug logging block (lines 44-52) prints to stderr every time a URI match fails. While useful for debugging, this could create noisy test output in normal test runs. Consider either removing this debug code, wrapping it in an environment variable check (e.g., if os.getenv("VCR_DEBUG")), or using Python's logging module with an appropriate log level instead of direct stderr printing.
remove unused parameter Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
copilot 한테 시켰더니 commit이 지저분해지고 되돌리기가 힘들다는 것을 알게됨. |
Fixes issues in the CD workflow that prevented successful PyPI releases.
Changes:
This ensures releases triggered by version tags can properly authenticate and publish to PyPI.
Prerequisites: Ensure
PYPI_API_TOKENis configured in repository secrets.