WIP: Update filer and taskmaster #59
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Update to alpine:3.22 (python 3.12) - Update tox - Update setup.py - Fix pylint
Reviewer's GuideModernizes Python/runtime support to 3.11–3.12, updates dependencies and Docker build flow to use alpine:3.23 and PEP 517 builds, and fixes minor linting/test issues in filer-related code. Flow diagram for updated multi-stage Docker build for filer and taskmasterflowchart TD
A[Start multi-stage Docker build] --> B[Builder stage based on alpine:3.23]
B --> C[Install python3 and py3-pip]
C --> D[Install git]
D --> E[Upgrade setuptools, pip, wheel, build]
E --> F[Set WORKDIR /app and copy source]
F --> G[Run python3 -m build --wheel]
G --> H[Remove sdist files from dist]
H --> I[Runtime stage based on alpine:3.23]
I --> J[Install python3 and py3-pip]
J --> K[Copy built wheel from builder]
K --> L[Install tesk_core wheel with dependencies]
L --> M[Image ready for filer or taskmaster]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The supported Python versions are now effectively 3.11–3.12 in tox and CI, but
python_requiresand the classifiers insetup.pystill claim support down to 3.5/3.7; consider tighteningpython_requiresand updating classifiers to match the actually tested versions. - The new
urllib3requirement only applies forpython_version >= "3.10"and nothing is specified for lower versions whilepython_requiresstill allows them; either add constraints for pre-3.10 or drop support for those versions so dependency resolution remains consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The supported Python versions are now effectively 3.11–3.12 in tox and CI, but `python_requires` and the classifiers in `setup.py` still claim support down to 3.5/3.7; consider tightening `python_requires` and updating classifiers to match the actually tested versions.
- The new `urllib3` requirement only applies for `python_version >= "3.10"` and nothing is specified for lower versions while `python_requires` still allows them; either add constraints for pre-3.10 or drop support for those versions so dependency resolution remains consistent.
## Individual Comments
### Comment 1
<location> `setup.py:11` </location>
<code_context>
with codecs.open(path.join(HERE, 'README.md'), encoding='utf-8') as f:
LONG_DESC = f.read()
-INSTALL_DEPS = ['kubernetes==9.0.0',
+INSTALL_DEPS = ['kubernetes==35.0.0',
'requests>=2.20.0',
</code_context>
<issue_to_address>
**issue (bug_risk):** Upgrading kubernetes dependency may require aligning supported Python versions with upstream requirements.
This is a large jump (`kubernetes==9.0.0` → `35.0.0`), and newer kubernetes client versions have dropped support for some older Python versions. Since `python_requires` still allows `>=3.5`, users on unsupported Python versions could hit install-time or runtime failures. Please either update `python_requires` to match the kubernetes client's supported versions or conditionally pin kubernetes by Python version.
</issue_to_address>
### Comment 2
<location> `setup.py:19-16` </location>
<code_context>
+ 'urllib3>=2.6.0,<3.0 ; python_version >= "3.10"',
- # boto3 constraint
- 'boto3<=1.28 ; python_version == "3.8"',
'boto3>=1.28,<2.0 ; python_version >= "3.9"',
]
TEST_DEPS = [ 'pytest',
</code_context>
<issue_to_address>
**issue (bug_risk):** boto3 constraint no longer mentions Python 3.8, while `python_requires` still allows it.
With `python_requires` still set to `>=3.5,<4.0`, a Python 3.8 environment can install this project but won’t match the new `boto3>=1.28,<2.0 ; python_version >= "3.9"` specifier, leaving boto3 effectively unconstrained. If 3.8 is no longer supported, please update `python_requires` (and classifiers) accordingly; if it is supported, consider reintroducing a 3.8-specific boto3 constraint or broadening the current one so 3.8 is covered explicitly.
</issue_to_address>
### Comment 3
<location> `setup.py:56` </location>
<code_context>
- 'License :: OSI Approved :: Apache Software License',
-
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.7'
],
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Classifier list and `python_requires` are out of sync with the new tox/GitHub Actions matrices.
Tox and the GitHub workflow only exercise Python 3.11 and 3.12, but the metadata still claims support for 3.7 and `python_requires='>=3.5, <4.0'`. This can mislead users and cause installs on unsupported versions, especially given the newer kubernetes/boto3/urllib3 constraints. Please align `python_requires` and classifiers with the versions you actually support (e.g., >=3.11, or the true minimum compatible version).
</issue_to_address>
### Comment 4
<location> `src/tesk_core/filer_s3.py:103` </location>
<code_context>
for obj in objects["Contents"]:
file_name = os.path.basename(obj["Key"])
dir_name = os.path.dirname(obj["Key"])
- path_to_create = re.sub(r'^' + self.file_path.strip('/').replace('/', '\/') + '', "", dir_name).strip('/')
+ path_to_create = re.sub(r'^' + self.file_path.strip('/').replace('/', r'\/') + '', "", dir_name).strip('/')
path_to_create = os.path.join(self.path, path_to_create)
os.makedirs(path_to_create, exist_ok=True)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Building the regex prefix by hand is fragile; `re.escape` would avoid issues with special characters.
This still manually builds a regex with `self.file_path.strip('/').replace('/', r'\/')`. If `self.file_path` contains other regex metacharacters (like `.` or `+`), the match can behave incorrectly. Please use `re.escape` for the dynamic prefix instead:
```python
prefix = re.escape(self.file_path.strip('/'))
path_to_create = re.sub(r'^' + prefix, '', dir_name).strip('/')
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for obj in objects["Contents"]: | ||
| file_name = os.path.basename(obj["Key"]) | ||
| dir_name = os.path.dirname(obj["Key"]) | ||
| path_to_create = re.sub(r'^' + self.file_path.strip('/').replace('/', '\/') + '', "", dir_name).strip('/') |
There was a problem hiding this comment.
suggestion (bug_risk): Building the regex prefix by hand is fragile; re.escape would avoid issues with special characters.
This still manually builds a regex with self.file_path.strip('/').replace('/', r'\/'). If self.file_path contains other regex metacharacters (like . or +), the match can behave incorrectly. Please use re.escape for the dynamic prefix instead:
prefix = re.escape(self.file_path.strip('/'))
path_to_create = re.sub(r'^' + prefix, '', dir_name).strip('/')...and update tox
trispera
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://endoflife.date/python
Summary by Sourcery
Update supported Python versions, dependencies, and container images while addressing linting issues and path handling.
Bug Fixes:
Enhancements: