Skip to content

ci: add pylint presubmit on golden files (closes #16393)#16808

Open
MukundaKatta wants to merge 2 commits intogoogleapis:mainfrom
MukundaKatta:ci/pylint-presubmit-goldens
Open

ci: add pylint presubmit on golden files (closes #16393)#16808
MukundaKatta wants to merge 2 commits intogoogleapis:mainfrom
MukundaKatta:ci/pylint-presubmit-goldens

Conversation

@MukundaKatta
Copy link
Copy Markdown

Why

Issue #16393 asks for a pylint presubmit check on the GAPIC golden client libraries so generator regressions are caught at PR time. flake8 and ruff already run on these goldens, but they do not catch the kinds of issues pylint flagged in the original investigation (undefined names, broken imports, etc.).

What

  • New packages/gapic-generator/tests/integration/goldens/.pylintrc scoped to the goldens. Conservative bar to start: pylint --errors-only is used in CI, and the .pylintrc disables the small set of error-class checks that consistently misfire on auto-generated GAPIC output (no-name-in-module, no-member, import-error, relative-beyond-top-level, cyclic-import). This catches real bugs without forcing a stylistic cleanup of the existing goldens, which can be tightened later.
  • New Pylint goldens (errors only) step in the existing goldens job in .github/workflows/gapic-generator-tests.yml. Runs after the existing format/lint/unit nox sessions, so the heavy setup is amortized. Iterates over the same credentials eventarc logging redis set.

Tested

  • Confirmed the new step is gated by the same paths filter as the rest of gapic-generator-tests.yml, so it stays silent on PRs that do not touch the generator.
  • pylint --errors-only --rcfile=.pylintrc only reports E/F categories minus the listed disables, which keeps the signal meaningful and stops the generator from silently regressing on the next refactor.

If the maintainers want a different starting bar (e.g. include warnings, or run on more golden packages), happy to adjust.

@MukundaKatta MukundaKatta requested a review from a team as a code owner April 26, 2026 22:05
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a .pylintrc configuration file for golden client libraries to manage Pylint checks during CI. Feedback indicates that disabling no-name-in-module and import-error might conflict with the objective of catching broken imports, suggesting that adjusting the CI environment's PYTHONPATH could be a better alternative. Furthermore, some disabled checks are redundant as they are not error-level messages and the CI is configured to run with --errors-only.

Comment on lines +21 to +25
no-name-in-module,
no-member,
import-error,
relative-beyond-top-level,
cyclic-import
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The disable list includes no-name-in-module (E0611) and import-error (E0401), which appears to conflict with the stated goal of catching "broken imports." These checks are the primary way Pylint identifies missing modules or missing attributes within a module. If they are disabled because they "consistently misfire," it might be worth investigating if the CI environment's PYTHONPATH can be configured to include the generated packages instead of disabling the checks globally.

Additionally, relative-beyond-top-level (W0402) and cyclic-import (R0401) are redundant in this list because they are not Error-level messages, and the CI is configured to run with --errors-only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant