Conversation
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.8", "3.9", "3.10"] |
There was a problem hiding this comment.
[BUG] The matrix targets Python versions this project does not support and omits the ones it does. setup.py declares python_requires=">=3.10, <=4.0, !=4.0", and .github/workflows/build.yml tests 3.10 through 3.14. Running on 3.8/3.9 means the job exercises interpreters the codebase is not expected to parse under (3.10+ syntax such as match statements and modern typing constructs), so those legs will fail for reasons unrelated to code quality, while 3.11–3.14 go unchecked.
Also, linting is interpreter-independent here — a single version is enough. Align with the rest of the repo:
strategy:
fail-fast: false
matrix:
python-version: ["3.10", "3.11", "3.12", "3.13", "3.14"]Or drop the matrix entirely and pin one version, as check_compatibility.yml and schema.yml do with python-version: "3.10".
| pip install pylint | ||
| - name: Analysing the code with pylint | ||
| run: | | ||
| pylint $(git ls-files '*.py') |
There was a problem hiding this comment.
[GENERAL] This step will fail on every run, permanently red-flagging the branch. Two independent reasons:
- No pylint configuration exists in the repo. There is no
.pylintrc,pylintrc, or[tool.pylint]section inpyproject.toml, so pylint runs with all default checks on all 420 tracked.pyfiles and exits non-zero on the first message. Defaults directly conflict with this repo's conventions:line-too-longat 100 chars vs. black'sline-length = 120,missing-module-docstring/missing-function-docstringacross the codebase, andinvalid-namefor existing files likebin/sam-translate.py,bin/json-format.py,bin/yaml-format.py, andtests/unit/model/api/TestSharedApiUsagePlan.py. - Project dependencies are never installed. The install step only installs pylint, so every third-party import (
boto3,jsonschema,pydantic,pytest, …) raisesE0401 import-error.
Beyond that, this largely duplicates existing tooling: ruff.toml already enables the pylint rule set via lint.select = [..., "PL", ...] with a tuned [lint.pylint] max-args = 6, and make lint runs ruff plus mypy --strict in build.yml.
If pylint is genuinely wanted alongside ruff, it needs a checked-in config plus the project installed, and it should go through the Makefile like every other check:
- run: make init
- run: pylint samtranslator bin schema_sourcewith a [tool.pylint] section in pyproject.toml disabling the checks ruff/black already own. Otherwise, consider whether this workflow adds value over the existing ruff configuration.
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install pylint |
There was a problem hiding this comment.
[GENERAL] pip install pylint is unpinned, so CI resolves whatever version is current at run time. Pylint adds and changes checks between releases, which means a new upload can turn this job red on a PR that changed nothing relevant — a failure mode this repo deliberately avoids by pinning every dev tool in requirements/dev.txt (ruff~=0.15.6, black==26.3.1, mypy~=1.10.1).
Add pylint to requirements/dev.txt with a bounded specifier and install it through make init so local runs and CI use the same version.
| @@ -0,0 +1,23 @@ | |||
| name: Pylint | |||
|
|
|||
| on: [push] | |||
There was a problem hiding this comment.
[GENERAL] on: [push] is inconsistent with every other workflow here and gives the wrong coverage. It fires on pushes to all branches (including personal/scratch branches, burning runner minutes) yet never runs on pull_request, so contributions from forks — the common case for this repo — are never linted. It also omits merge_group, which build.yml, codeql.yml, and check_compatibility.yml all declare; without it this check can never be satisfied in the merge queue if it is ever made required.
Match the existing pattern:
on:
push:
branches:
- develop
- "feat-*"
pull_request:
merge_group:
types: [checks_requested]|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
[SECURITY] No permissions block is declared, so the job inherits the repository-wide default GITHUB_TOKEN scope, which can be read/write on all scopes. A linting job only needs to read the checked-out source, and this workflow installs a package from PyPI and runs it over the repository — keeping the token minimal limits what a compromised or malicious dependency could do with it. Both codeql.yml and schema.yml already scope their tokens explicitly.
jobs:
build:
runs-on: ubuntu-latest
permissions:
contents: read
|
Hi, if there's a specific use case please open a issue as feature request instead. |
2 participants
Issue #, if available
Description of changes
Description of how you validated changes
Checklist
Examples?
Please reach out in the comments if you want to add an example. Examples will be
added to
sam initthrough aws/aws-sam-cli-app-templates.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.