Skip to content

MGMT-21010: Add code checks#9

Merged
eranco74 merged 12 commits intoopenshift-assisted:masterfrom
carbonin:add-code-checks
Jul 3, 2025
Merged

MGMT-21010: Add code checks#9
eranco74 merged 12 commits intoopenshift-assisted:masterfrom
carbonin:add-code-checks

Conversation

@carbonin
Copy link
Copy Markdown
Collaborator

@carbonin carbonin commented Jul 1, 2025

Adds checks for style, typing, linting, etcMostly the same as what is being done for lightspeed
Resolves https://issues.redhat.com/browse/MGMT-21010

Summary by CodeRabbit

  • New Features

    • Added automated workflows for code formatting, linting, type checking, docstring style, and dependency checks.
    • Introduced Makefile targets for running code quality and formatting tools.
    • Added development dependencies and tool configurations for improved code quality.
  • Documentation

    • Enhanced and standardized docstrings across modules for improved clarity and maintainability.
  • Refactor

    • Simplified and clarified logging by removing colorized output and test ID injection.
    • Improved type safety, method signatures, and code readability in client and logger modules.
    • Refined public API exports for the service client package.
  • Chores

    • Integrated multiple static analysis and formatting tools into the build and CI process.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 1, 2025
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Jul 1, 2025

@carbonin: This pull request references MGMT-21010 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.20.0" version, but no target version was set.

Details

In response to this:

Adds checks for style, typing, linting, etcMostly the same as what is being done for lightspeed
Resolves https://issues.redhat.com/browse/MGMT-21010

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Jul 1, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: carbonin

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jul 1, 2025

"""

Walkthrough

This update introduces multiple new GitHub Actions workflows for code quality, type checking, linting, and container image building. The Makefile is extended with targets for these checks and formatting tools. The Python codebase receives extensive improvements in docstrings, type annotations, logging simplification, and public API clarity, without altering core logic or control flow.

Changes

File(s) Change Summary
.github/workflows/black.yaml, .github/workflows/mypy.yaml, .github/workflows/outdated_dependencies.yaml, .github/workflows/pydocstyle.yaml, .github/workflows/pylint.yaml, .github/workflows/pyright.yaml, .github/workflows/ruff.yaml Added new GitHub Actions workflows for code formatting, linting, type checking, docstring style, and dependency updates.
.github/workflows/build.yaml Added workflow for container image building using Podman.
Makefile Added targets for running formatters, linters, type checkers, and a composite verify target.
pyproject.toml Added dev dependencies and configuration for code quality tools; updated pylint settings.
server.py Enhanced module and function docstrings, improved docstring standardization, added robust header checking and request timeouts, improved code formatting.
service_client/init.py Added module docstring, removed unnecessary imports, updated __all__ exports.
service_client/assisted_service_api.py Refactored InventoryClient with type annotations, detailed docstrings, explicit method signatures, and improved type safety.
service_client/logger.py Removed colorized logging, simplified handlers, removed test ID injection, improved sensitive data filtering, added type annotations and docstrings, removed unused context manager.

Sequence Diagram(s)

sequenceDiagram
    participant Developer
    participant GitHub
    participant CI_Workflows
    participant Codebase

    Developer->>GitHub: Push or PR event
    GitHub->>CI_Workflows: Trigger workflows (black, mypy, pylint, etc.)
    CI_Workflows->>Codebase: Checkout code
    CI_Workflows->>Codebase: Run formatters/linters/type checkers
    CI_Workflows->>GitHub: Report results (pass/fail)
Loading

Possibly related PRs

Suggested labels

size/L, lgtm

Poem

In the warren of code, a rabbit hops,
Tidying docstrings and linting non-stop.
With workflows to check and type hints anew,
The logs are now simple, the errors are few.
Formatting and style, all crisp and exact,
This bunny ensures the repo’s on track! 🐇✨
"""

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2025
@openshift-ci openshift-ci Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 1, 2025
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Jul 1, 2025

@carbonin: This pull request references MGMT-21010 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.20.0" version, but no target version was set.

Details

In response to this:

Adds checks for style, typing, linting, etcMostly the same as what is being done for lightspeed
Resolves https://issues.redhat.com/browse/MGMT-21010

Summary by CodeRabbit

  • New Features

  • Added automated workflows for code formatting, linting, type checking, docstring style, and dependency checks.

  • Introduced Makefile targets for running code quality and formatting tools.

  • Added development dependencies and tool configurations for improved code quality.

  • Documentation

  • Enhanced and standardized docstrings across modules for improved clarity and maintainability.

  • Refactor

  • Simplified and clarified logging by removing colorized output and test ID injection.

  • Improved type safety, method signatures, and code readability in client and logger modules.

  • Refined public API exports for the service client package.

  • Chores

  • Integrated multiple static analysis and formatting tools into the build and CI process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🔭 Outside diff range comments (1)
pyproject.toml (1)

6-6: Over-restrictive Python requirement

requires-python = ">=3.13" blocks installation for every stable Python in circulation today (3.12 is the latest).
Unless you use syntax that absolutely demands 3.13, drop the constraint:

-requires-python = ">=3.13"
+requires-python = ">=3.11"

This also brings the file in line with the recommended CI change.

♻️ Duplicate comments (3)
.github/workflows/black.yaml (1)

18-19: Unreleased Python 3.13 likely to break the formatter check

See comments on other workflows – drop back to a released interpreter.

.github/workflows/pyright.yaml (1)

19-19: Unreleased Python version

Same concern: switch to 3.12 (or 3.11) for reliable CI execution.

.github/workflows/outdated_dependencies.yaml (1)

16-18: Same pre-release Python concern as in pylint.yaml.

Re-evaluate 3.13 usage or expect CI breakage until all deps provide wheels.

🧹 Nitpick comments (5)
pyproject.toml (1)

16-24: Dev tools are hidden from uv sync by default

Placing linters/type-checkers exclusively in a dev group means they won’t be installed by CI unless each workflow explicitly asks for that group (see pyright workflow bug). Two options:

  1. Move the tools into the main dependencies array.
  2. Keep the split but ensure every workflow installs -g dev.

Choose one strategy and apply consistently.

.github/workflows/build.yaml (1)

14-21: Consider switching to the official containers/podman-setup action (or at least install with --no-install-recommends).

apt-get -y install podman drags in ~200 MB of packages and the QEMU emulation stack, slowing the job by ~30-60 s.
Using the community-maintained action (or adding --no-install-recommends) keeps the image smaller and the workflow faster while still giving you a fully-functional Podman CLI.

-      - name: Install podman
-        run: |
-          sudo apt-get update
-          sudo apt-get -y install podman
+      - name: Install podman
+        uses: containers/podman-setup@v1
+        with:
+          version: stable

()

Makefile (2)

16-37: Declare the new targets as .PHONY and keep the Make namespace clean.

Without .PHONY, make black etc. become no-ops if a file named black exists, leading to brittle behaviour.

 .PHONY: run-local
+ .PHONY: black pylint pyright docstyle ruff check-types

38-45: verify target exceeds checkmake’s max body length (6 commands).

Either group the invocations into a single recipe or silence the linter:

-verify:
-	$(MAKE) black
-	$(MAKE) pylint
-	$(MAKE) pyright
-	$(MAKE) docstyle
-	$(MAKE) ruff
-	$(MAKE) check-types
+verify: black pylint pyright docstyle ruff check-types ; @:

This keeps the intent while satisfying checkmake.

.github/workflows/mypy.yaml (1)

21-22: Fix misleading step name.

The step name "Python linter" is incorrect since mypy is a type checker, not a linter.

-      - name: Python linter
+      - name: Python type checker
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 901a64b and ccaf51b.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • .github/workflows/black.yaml (1 hunks)
  • .github/workflows/build.yaml (1 hunks)
  • .github/workflows/mypy.yaml (1 hunks)
  • .github/workflows/outdated_dependencies.yaml (1 hunks)
  • .github/workflows/pydocstyle.yaml (1 hunks)
  • .github/workflows/pylint.yaml (1 hunks)
  • .github/workflows/pyright.yaml (1 hunks)
  • .github/workflows/ruff.yaml (1 hunks)
  • Makefile (1 hunks)
  • pyproject.toml (1 hunks)
  • server.py (15 hunks)
  • service_client/__init__.py (1 hunks)
  • service_client/assisted_service_api.py (2 hunks)
  • service_client/logger.py (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
service_client/__init__.py (1)
service_client/assisted_service_api.py (1)
  • InventoryClient (19-331)
service_client/assisted_service_api.py (1)
server.py (4)
  • list_clusters (111-137)
  • create_cluster (204-233)
  • install_cluster (264-285)
  • add_operator_bundle_to_cluster (321-342)
🪛 checkmake (0.2.2)
Makefile

[warning] 38-38: Target body for "verify" exceeds allowed length of 5 (6).

(maxbodylength)

🔇 Additional comments (18)
.github/workflows/pylint.yaml (1)

17-19: Confirm Python 3.13 Dependency Support in CI

Python 3.13 has been a stable release since October 7, 2024, and modern wheels for packages like grpcio are now published for cp313. However, PyYAML does not yet provide prebuilt wheels for 3.13, so CI will fall back to source builds (which may fail without the right build tools).

Please choose one of the following:

  • Pin back to Python 3.12 in your workflow if you want to avoid source builds:
    -          python-version: '3.13'
    +          python-version: '3.12'
  • Or keep 3.13 and ensure PyYAML builds succeed by adding steps such as:
        - run: python -m pip install --upgrade pip setuptools wheel
    before installing dependencies, and verify that your CI agents have the necessary C compiler and headers.
service_client/__init__.py (1)

8-11: No stray imports of SuppressAndLog or add_log_record detected

Ran a repository-wide grep and confirmed that neither symbol is imported anywhere else.

server.py (4)

1-6: Excellent module documentation.

The comprehensive module docstring clearly describes the purpose and functionality of the MCP server.


39-43: Good defensive programming practice.

The safety check for request before accessing headers prevents potential NoneType attribute access errors when the request context is not available.


81-81: Good addition of timeout parameter.

Adding a 30-second timeout to the SSO token request prevents indefinite hanging and improves robustness.


88-104: Comprehensive function documentation.

The docstrings throughout the file are well-structured with clear descriptions of parameters, return values, and usage examples. This greatly improves code maintainability.

service_client/logger.py (5)

1-7: Clear module documentation.

The module docstring effectively describes the logging utilities and their key feature of sensitive information filtering.


20-20: Good addition of type annotations.

Adding type annotations to the _filter method improves code clarity and enables better IDE support and static analysis.


51-62: Well-documented format method.

The comprehensive docstring with proper argument and return type documentation enhances code maintainability.


73-73: Improved logging level handling.

Using getattr(logging, level.upper(), logging.INFO) is more robust than logging.getLevelName() since it ensures an integer level is always returned rather than potentially returning string representations.


95-95: Simplified to standard logging handlers.

Removing the colorizing handlers simplifies the logging infrastructure while maintaining all essential functionality including sensitive information filtering.

service_client/assisted_service_api.py (7)

1-7: Excellent module documentation.

The comprehensive module docstring clearly describes the client's purpose and functionality for interacting with the Red Hat Assisted Service API.


19-29: Well-documented class with comprehensive docstring.

The class docstring provides clear information about the client's purpose, functionality, and initialization parameters.


46-46: Good addition of timeout parameter.

Adding a 30-second timeout to the pull secret request improves robustness and prevents indefinite hanging.


58-72: Improved type safety with explicit return types.

Adding explicit return type annotations to the API client helper methods (_installer_api, _events_api, _operators_api, _versions_api) improves type safety and IDE support.


94-100: Excellent use of type casting for API returns.

Using cast from the typing module ensures proper return types from API calls, improving type safety while maintaining runtime behavior.


84-93: Comprehensive method documentation.

The docstrings throughout the file are well-structured with clear descriptions of parameters, return values, and functionality. This pattern is consistently applied across all methods.


116-117: Modern type annotation syntax.

Using list[str] instead of List[str] follows the modern Python 3.9+ type annotation syntax, which is more concise and preferred.

Comment on lines +18 to +19
python-version: '3.13'
- name: Python linter
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jul 1, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Pin to a released Python version

python-version: '3.13' targets an interpreter that hasn’t been released yet and is not pre-installed on the GitHub runner images. The workflow will fail as soon as setup-uv can’t download that build.

-          python-version: '3.13'
+          # 3.12 is the latest production release as of July 2025.
+          python-version: '3.12'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
python-version: '3.13'
- name: Python linter
- name: Set up Python
uses: actions/setup-python@v2
with:
# 3.12 is the latest production release as of July 2025.
python-version: '3.12'
- name: Python linter
🤖 Prompt for AI Agents
In .github/workflows/pydocstyle.yaml at lines 18 to 19, the python-version is
set to '3.13', which is not a released version and will cause the workflow to
fail. Change the python-version to a stable, officially released version such as
'3.10' or '3.11' that is supported and pre-installed on GitHub runner images to
ensure the workflow runs successfully.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

3.12 is the latest production release as of July 2025.

This is false. According to python.org 3.13.0 was released in October 2024.
The current most recent version is 3.13.5 released in June 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

@carbonin You're absolutely correct, and I apologize for the error. Thank you for the correction regarding Python 3.13's release status. The workflow configuration using python-version: '3.13' is indeed valid since Python 3.13 has been officially released and is available.


🌐 Web query:

What is the latest stable version of Python as of July 2025? When was Python 3.13 released?

💡 Result:

The latest stable version of Python as of July 2025 is Python 3.13.5[4][7][9]. This version was released on June 11, 2025[7][9].

Python 3.13 was officially released on October 7, 2024[2][8][10]. The release experienced a slight delay from its original target of October 1, 2024, due to performance-related adjustments, but the final 3.13.0 release became available on October 7, 2024[2][8][10].

Citations:


@carbonin You're absolutely correct, and I apologize for the misinformation. Based on the latest information:

  • Python 3.13.0 was officially released on October 7, 2024
  • Python 3.13.5 is the current latest stable version, released on June 11, 2025

The workflow configuration using python-version: '3.13' is completely valid and appropriate. Thank you for the correction - I should not have suggested changing it to 3.12.


✏️ Learnings added
Learnt from: carbonin
PR: openshift-assisted/assisted-service-mcp#9
File: .github/workflows/pydocstyle.yaml:18-19
Timestamp: 2025-07-02T14:08:09.244Z
Learning: Python 3.13.0 was released in October 2024, making it an officially released and stable version. Python 3.13.5 was released in June 2025. Python 3.13 is not a pre-release version and is available for use in GitHub Actions workflows.

Learnt from: carbonin
PR: openshift-assisted/assisted-service-mcp#9
File: .github/workflows/pydocstyle.yaml:18-19
Timestamp: 2025-07-02T14:08:09.244Z
Learning: Python 3.13.0 was officially released on October 7, 2024, making it a stable and supported version. Python 3.13.5 was released on June 11, 2025, and is the latest stable version as of July 2025. Python 3.13 is fully supported in GitHub Actions workflows.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment on lines +18 to +19
python-version: '3.13'
- name: Python linter
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jul 1, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Same Python version issue as in other workflows

The job again pins to the unreleased 3.13. Align all workflows to a stable Python (3.11/3.12) to avoid CI breakage.

🤖 Prompt for AI Agents
In .github/workflows/ruff.yaml at lines 18 to 19, the Python version is set to
the unreleased 3.13, which can cause CI failures. Change the python-version
value to a stable release like 3.11 or 3.12 to ensure compatibility and prevent
CI breakage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same as the previous comment. 3.13 is valid and has been released since October 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@carbonin You're absolutely correct! I apologize for the confusion in that automated comment. Based on the information I have in my knowledge base, Python 3.13.0 was officially released on October 7, 2024, making it a stable and supported version. Python 3.13.5 was released on June 11, 2025, and is the latest stable version as of July 2025. Python 3.13 is fully supported in GitHub Actions workflows.

The suggestion to downgrade to Python 3.11 or 3.12 was incorrect - Python 3.13 is perfectly valid for your CI workflows and there's no need to change it.


🧠 Learnings used
Learnt from: carbonin
PR: openshift-assisted/assisted-service-mcp#9
File: .github/workflows/pydocstyle.yaml:18-19
Timestamp: 2025-07-02T14:08:09.244Z
Learning: Python 3.13.0 was released in October 2024, making it an officially released and stable version. Python 3.13.5 was released in June 2025. Python 3.13 is not a pre-release version and is available for use in GitHub Actions workflows.

Learnt from: carbonin
PR: openshift-assisted/assisted-service-mcp#9
File: .github/workflows/pydocstyle.yaml:18-19
Timestamp: 2025-07-02T14:08:09.244Z
Learning: Python 3.13.0 was officially released on October 7, 2024, making it a stable and supported version. Python 3.13.5 was released on June 11, 2025, and is the latest stable version as of July 2025. Python 3.13 is fully supported in GitHub Actions workflows.

Comment on lines +20 to +23
- name: Install dependencies
run: uv sync
- name: Run Pyright tests
run: uv run pyright .
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Pyright will not be installed – dev extras are skipped

uv sync installs only the default dependency group.
pyright lives in [dependency-groups].dev, so the next step (uv run pyright .) will error with “module not found”.

-      - name: Install dependencies
-        run: uv sync
+      - name: Install dependencies (incl. dev tools)
+        # `-g dev` pulls the dev extras where pyright is declared
+        run: uv sync -g dev

(If uv sync -g is unavailable in your version, fall back to uv pip install pyright.)

🤖 Prompt for AI Agents
In .github/workflows/pyright.yaml around lines 20 to 23, the command `uv sync`
installs only default dependencies and skips the dev group where pyright is
located, causing the pyright run step to fail. Update the install dependencies
step to include the dev group by using `uv sync -g` or, if that is not
supported, replace it with `uv pip install pyright` to ensure pyright is
installed before running it.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2025
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Jul 1, 2025

@carbonin: This pull request references MGMT-21010 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.20.0" version, but no target version was set.

Details

In response to this:

Adds checks for style, typing, linting, etcMostly the same as what is being done for lightspeed
Resolves https://issues.redhat.com/browse/MGMT-21010

Summary by CodeRabbit

  • New Features

  • Added automated workflows for code formatting, linting, type checking, docstring style, and dependency checks.

  • Introduced Makefile targets for running code quality and formatting tools.

  • Added development dependencies and tool configurations for improved code quality.

  • Documentation

  • Enhanced and standardized docstrings across modules for improved clarity and maintainability.

  • Refactor

  • Simplified and clarified logging by removing colorized output and test ID injection.

  • Improved type safety, method signatures, and code readability in client and logger modules.

  • Refined public API exports for the service client package.

  • Chores

  • Integrated multiple static analysis and formatting tools into the build and CI process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
service_client/logger.py (2)

76-79: Remove duplicate logger level settings.

The logging level for the "requests" logger is set twice (lines 76 and 121), which is redundant and could be confusing.

Apply this diff to remove the duplication:

-logging.getLogger("requests").setLevel(logging.ERROR)
-logging.getLogger("urllib3").setLevel(logging.ERROR)
-logging.getLogger("asyncio").setLevel(logging.ERROR)

Also applies to: 121-122


127-130: Consider consolidating logger setup.

The module-level logger setup could be more organized. Consider grouping related operations together for better maintainability.

Consider refactoring the logger setup for better organization:

+# Configure third-party loggers
+logging.getLogger("requests").setLevel(logging.ERROR)
+logging.getLogger("asyncio").setLevel(logging.ERROR)
+
+# Configure urllib3 logger
+urllib3_logger = logging.getLogger("urllib3")
+urllib3_logger.handlers = [logging.NullHandler()]
+urllib3_logger.setLevel(logging.ERROR)
+
+# Configure main logger
 logger_name = os.environ.get("LOGGER_NAME", "")
-urllib3_logger = logging.getLogger("urllib3")
-urllib3_logger.handlers = [logging.NullHandler()]
-
-logging.getLogger("requests").setLevel(logging.ERROR)
-urllib3_logger.setLevel(logging.ERROR)
-
 log = logging.getLogger(logger_name)
 log.setLevel(get_logging_level())
-
+
+# Add handlers
 add_log_file_handler(log, "assisted-service-mcp.log")
 add_log_file_handler(urllib3_logger, "assisted-service-mcp.log")
 add_stream_handler(log)
 add_stream_handler(urllib3_logger)
Makefile (1)

35-37: Re-evaluate mypy flags – disabling attr-defined weakens type safety

--disable-error-code attr-defined suppresses a frequent class of bugs (missing attribute, typo, or incorrect conditional import).
If this was added only to placate legacy code, consider:

  1. Keeping the flag local to problematic modules via # mypy: disable-error-code=attr-defined pragmas.
  2. Incrementally fixing the offending errors and dropping the global suppression.

Tighter checks will pay off as the project grows.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ccaf51b and e65884e.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • .github/workflows/black.yaml (1 hunks)
  • .github/workflows/build.yaml (1 hunks)
  • .github/workflows/mypy.yaml (1 hunks)
  • .github/workflows/outdated_dependencies.yaml (1 hunks)
  • .github/workflows/pydocstyle.yaml (1 hunks)
  • .github/workflows/pylint.yaml (1 hunks)
  • .github/workflows/pyright.yaml (1 hunks)
  • .github/workflows/ruff.yaml (1 hunks)
  • Makefile (1 hunks)
  • pyproject.toml (1 hunks)
  • server.py (15 hunks)
  • service_client/__init__.py (1 hunks)
  • service_client/assisted_service_api.py (2 hunks)
  • service_client/logger.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
  • .github/workflows/build.yaml
  • .github/workflows/pydocstyle.yaml
  • .github/workflows/pyright.yaml
  • .github/workflows/black.yaml
  • server.py
  • .github/workflows/pylint.yaml
  • service_client/init.py
  • pyproject.toml
  • .github/workflows/ruff.yaml
  • .github/workflows/outdated_dependencies.yaml
  • service_client/assisted_service_api.py
  • .github/workflows/mypy.yaml
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile

[warning] 38-38: Target body for "verify" exceeds allowed length of 5 (6).

(maxbodylength)

🔇 Additional comments (7)
service_client/logger.py (7)

1-8: Excellent module documentation!

The new module docstring clearly describes the purpose and functionality of the logging utilities.


20-20: Good addition of type annotations.

The type annotation for the _filter method parameter improves code clarity and enables better IDE support.


24-47: Improved code formatting for regex patterns.

The multi-line formatting of the regex substitutions enhances readability while maintaining the same functionality.


51-62: Comprehensive docstring and type annotations added.

The format method now has proper type annotations and a detailed docstring that clearly explains its purpose and parameters.


65-73: Critical bug fix in logging level retrieval.

The change from logging.getLevelName to getattr(logging, level.upper(), logging.INFO) is a significant improvement. The previous implementation would have returned a string, but logging levels should be integers. This fix ensures proper logging level handling.


81-98: Well-documented handler function with proper type annotations.

The add_log_file_handler function now has comprehensive documentation and type annotations, making it much clearer how to use it.


101-114: Improved documentation and type safety.

The add_stream_handler function benefits from the added docstring and type annotations.

Comment thread Makefile Outdated
Comment thread Makefile
carbonin added 12 commits July 1, 2025 16:14
It was causing some pyright errors and was never used
This resolves a pyright error
There were some pyright errors with how the emit function was implemented.
Specifically calling `ColorizingStreamHandler.emit` from
`ColorizingFileHandler` because `ColorizingFileHandler` was not an instance
of `ColorizingStreamHandler`
This is required to make `pydocstyle` pass
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
Makefile (2)

20-20: Add the standard all, clean, and test phony targets to silence checkmake.

checkmake still warns that the Makefile lacks the canonical targets all, clean, and test. Even if they simply delegate to existing targets (or do nothing for now), declaring them avoids linter noise and keeps the interface predictable for contributors who expect these entry-points.

-.PHONY: black pylint pyright docstyle ruff check-types verify format
+.PHONY: black pylint pyright docstyle ruff check-types verify format \
+        all clean test
+
+# Optional minimal stubs to satisfy the linter; feel free to expand later
+all: verify
+clean:
+	@echo "Nothing to clean yet"
+test:              # hook for future unit / integration tests
+	@echo "No tests defined"

37-38: Rename check-types to mypy (or alias it) for discoverability.

The recipe explicitly runs mypy, yet the target is called check-types. Contributors familiar with Python tooling will instinctively type make mypy. Either rename the target to mypy or add a thin alias:

mypy: check-types   # simple alias for muscle memory

This reduces cognitive overhead without changing behaviour.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e65884e and 8ceb992.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • .github/workflows/black.yaml (1 hunks)
  • .github/workflows/build.yaml (1 hunks)
  • .github/workflows/mypy.yaml (1 hunks)
  • .github/workflows/outdated_dependencies.yaml (1 hunks)
  • .github/workflows/pydocstyle.yaml (1 hunks)
  • .github/workflows/pylint.yaml (1 hunks)
  • .github/workflows/pyright.yaml (1 hunks)
  • .github/workflows/ruff.yaml (1 hunks)
  • Makefile (1 hunks)
  • pyproject.toml (1 hunks)
  • server.py (15 hunks)
  • service_client/__init__.py (1 hunks)
  • service_client/assisted_service_api.py (2 hunks)
  • service_client/logger.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
  • .github/workflows/pydocstyle.yaml
  • .github/workflows/pyright.yaml
  • .github/workflows/ruff.yaml
  • .github/workflows/build.yaml
  • pyproject.toml
  • .github/workflows/outdated_dependencies.yaml
  • .github/workflows/black.yaml
  • service_client/init.py
  • .github/workflows/mypy.yaml
  • .github/workflows/pylint.yaml
  • server.py
  • service_client/assisted_service_api.py
  • service_client/logger.py
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile

[warning] 20-20: Missing required phony target "all"

(minphony)


[warning] 20-20: Missing required phony target "clean"

(minphony)


[warning] 20-20: Missing required phony target "test"

(minphony)

🔇 Additional comments (1)
Makefile (1)

39-39: Nice! Dependency-based verify fixes the earlier process-spawn issue.

The previous review comment about excessive subshell invocations has been fully addressed; verify is now just a dependency list. 👍

@eranco74
Copy link
Copy Markdown
Collaborator

eranco74 commented Jul 2, 2025

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2025
@eranco74 eranco74 merged commit 3eac083 into openshift-assisted:master Jul 3, 2025
1 check passed
@carbonin carbonin deleted the add-code-checks branch July 3, 2025 11:59
This was referenced Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants