Skip to content

Add Pylint workflow for Python code analysis #3992

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.

Sign up for GitHub

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

Closed
bmchains wants to merge 1 commit into from
+23 −0
Closed

Add Pylint workflow for Python code analysis #3992

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/pylint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: Pylint

on: [push]

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Sep 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Sep 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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

strategy:
matrix:
python-version: ["3.8", "3.9", "3.10"]

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Sep 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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".

steps:
- uses: actions/checkout@v4
Comment thread
bmchains marked this conversation as resolved.
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v3
with:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install pylint

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Sep 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

- name: Analysing the code with pylint
run: |
pylint $(git ls-files '*.py')

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Sep 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[GENERAL] This step will fail on every run, permanently red-flagging the branch. Two independent reasons:

  1. No pylint configuration exists in the repo. There is no .pylintrc, pylintrc, or [tool.pylint] section in pyproject.toml, so pylint runs with all default checks on all 420 tracked .py files and exits non-zero on the first message. Defaults directly conflict with this repo's conventions: line-too-long at 100 chars vs. black's line-length = 120, missing-module-docstring/missing-function-docstring across the codebase, and invalid-name for existing files like bin/sam-translate.py, bin/json-format.py, bin/yaml-format.py, and tests/unit/model/api/TestSharedApiUsagePlan.py.
  2. Project dependencies are never installed. The install step only installs pylint, so every third-party import (boto3, jsonschema, pydantic, pytest, …) raises E0401 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_source

with 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.