Skip to content

Conversation

@MrBr-github
Copy link

Issue: HPCINFRA-3592

What

Make sure that documentation auto generation wasn't impacted by PR changes

How ?

Create documentation testing procedure

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Check list

  • Code follows the style de facto guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

@MrBr-github MrBr-github force-pushed the ci_docs_test branch 2 times, most recently from 31c9650 to 3382787 Compare August 5, 2025 10:35
@MrBr-github
Copy link
Author

Currently execution of python3 generate_docs.py generates an unclear error

# python3 generate_docs.py
Error: 'bool' object has no attribute 'get'

After adding below patch it seems that there is an issue in the code

# python3 generate_docs.py
Error processing property performance.rings.tx.additionalProperties: 'bool' object has no attribute 'get'

@AlexanderGrissik @galnoam @tomerdbz
How do you want to proceed before we merge this PR?

  • Fix performance.rings.tx.additionalProperties issue?
  • Enhance generate_docs.py verbosity, currently it fails to produce meaningful errors?

generate_docs.py patch

$ git diff
diff --git a/generate_docs.py b/generate_docs.py
index dda4d5b..12ed64b 100644
--- a/generate_docs.py
+++ b/generate_docs.py
@@ -119,10 +119,14 @@ class SchemaProcessor:
             self.processed_props.add(current_path)

             # Extract property information
-            description = prop_data.get("description", "")
-            default = prop_data.get("default", None)
-            env_var = ConfigPropertyParser.extract_env_var(description)
-
+            try:
+                description = prop_data.get("description", "")
+                default = prop_data.get("default", None)
+                env_var = ConfigPropertyParser.extract_env_var(description)
+            except Exception as e:
+                print(f"Error processing property {current_path}: {e}")
+                sys.exit(1)
+
             # If this is an object with properties, process its children
             if prop_data.get("type") == "object":
                 # Create an entry for this section

@AlexanderGrissik
Copy link
Collaborator

The usage of additionalProperties is unclear. It is false for all the schema. Waiting for @tomerdbz to evaluate and fix .

@tomerdbz
Copy link
Collaborator

you have to say in a schema whether you accept additional properties per namespace.
e.g. - if core.resources won't have "additionalProperties: false" a valid config could also contain core.resources.i_love_grissik

and no one will yell on this - i_love_grissik simply will be ignored.

anyway - there's a bug here in the python - I'll apply the patch - it's a good idea to have extended error info and fix it. on it :)

@MrBr-github MrBr-github requested a review from dpressle October 19, 2025 15:32
Copy link
Collaborator

@dpressle dpressle left a comment

Choose a reason for hiding this comment

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

i need to see with this new step

@MrBr-github
Copy link
Author

MrBr-github commented Oct 20, 2025

rebased code on vNext due to merge conflict and force pushed

@MrBr-github
Copy link
Author

bot:retest

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR adds automated documentation generation validation to the CI pipeline. It introduces a new do_documentation_test boolean parameter (default: true) in the Jenkins job configuration and a corresponding "Documentation Test" step in the matrix pipeline that executes generate_docs.py and generate_mappings.py. The test runs in a containerized ubuntu22.04 environment without requiring physical agents, following the same pattern as other static analysis steps. The implementation ensures that PRs cannot break the project's auto-generated documentation (configuration mappings, etc.) by catching generation failures early in the CI process. The change integrates cleanly with the existing Autotools-based build system and declarative YAML pipeline architecture described in the .ci/ directory structure.

Important Files Changed

Filename Score Overview
.ci/opensource_jjb.yaml 5/5 Added do_documentation_test boolean parameter with default true to control documentation validation in CI
.ci/matrix_job.yaml 4/5 Added documentation_test container definition and "Documentation Test" step that runs Python doc generation scripts

Confidence score: 4/5

  • This PR is safe to merge with minimal risk of breaking production functionality
  • Score reflects straightforward CI additions following established patterns, but deducted one point because: (1) no artifact archiving may complicate debugging failed documentation generation, (2) older Docker tag (20250219) versus other ubuntu22.04 images could cause subtle dependency issues, and (3) no explicit error-handling verification for the Python scripts
  • Pay close attention to .ci/matrix_job.yaml to ensure the documentation test step fails appropriately when generation breaks

Sequence Diagram

sequenceDiagram
    participant User
    participant GitHub
    participant Jenkins
    participant Docker
    participant Kubernetes
    participant Build
    participant Tests
    participant Artifacts

    User->>GitHub: Create/Update PR
    GitHub->>Jenkins: Trigger pipeline (bot:retest)
    Jenkins->>Jenkins: Load matrix_job.yaml config
    Jenkins->>Jenkins: Parse job parameters (do_documentation_test, etc.)
    Jenkins->>Docker: Build/pull Docker images
    Docker-->>Jenkins: Images ready
    Jenkins->>Kubernetes: Create privileged pod
    Kubernetes-->>Jenkins: Pod ready
    Jenkins->>Build: Execute Setup step
    Jenkins->>Build: Install DOCA host
    Jenkins->>Build: Run Copyrights check
    Jenkins->>Build: Execute Autogen
    Jenkins->>Build: Execute Build step
    alt Build fails
        Build->>Artifacts: Collect failure artifacts
        Artifacts-->>Jenkins: Archive arch-*.tar.gz
    end
    Jenkins->>Tests: Execute Style check
    Jenkins->>Tests: Execute Compiler check
    Jenkins->>Tests: Execute Coverity
    Jenkins->>Tests: Execute Cppcheck
    Jenkins->>Tests: Execute Csbuild
    Jenkins->>Tests: Execute Unit Tests
    Jenkins->>Tests: Execute Gtest
    Jenkins->>Tests: Execute Valgrind
    Jenkins->>Tests: Execute Documentation Test
    Tests->>Tests: Run "python3 generate_docs.py"
    Tests->>Tests: Run "python3 src/core/config/generate_mappings.py"
    alt Test fails
        Tests->>Artifacts: Collect failure artifacts
        Artifacts-->>Jenkins: Archive logs and reports
    end
    Jenkins->>Artifacts: Collect all artifacts
    Artifacts->>Artifacts: Compress artifacts with .ci/artifacts.sh
    Artifacts-->>Jenkins: Archive jenkins/**/arch-*.tar.gz
    Jenkins->>Tests: Execute Blackduck scan
    Jenkins->>Jenkins: Pipeline stop
    Jenkins->>GitHub: Update PR status
    GitHub->>User: Display test results
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@MrBr-github
Copy link
Author

bot:retest

@MrBr-github
Copy link
Author

@galnoam
please merge

Make sure that documentation auto generation wasn't impacted by PR
changes

Create documentation testing procedure

Issue: HPCINFRA-3592

Signed-off-by: Michael Braverman <[email protected]>
@MrBr-github
Copy link
Author

Squashed the commits

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Added CI integration to validate documentation generation isn't broken by code changes. The PR introduces a new documentation test step that runs two Python scripts (generate_docs.py and generate_mappings.py) to verify auto-generated documentation can be successfully created.

Key Changes:

  • New documentation_test Docker container using Ubuntu 22.04 build stage
  • New "Documentation Test" CI step that executes both documentation generation scripts
  • New do_documentation_test boolean parameter (default: true) to control test execution
  • Follows existing CI patterns (containerSelector, agentSelector, enable flag)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-structured CI additions that follow established patterns in the codebase. Both scripts exist, use only standard library imports (json, re, sys, pathlib, typing), and the required schema file is present. The Docker target 'build' exists in the Dockerfile. The implementation mirrors existing test steps like Unit Tests.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
.ci/matrix_job.yaml 5/5 Adds documentation_test container and Documentation Test step to verify auto-generated docs
.ci/opensource_jjb.yaml 5/5 Adds do_documentation_test boolean parameter with default true to enable/disable documentation tests

Sequence Diagram

sequenceDiagram
    participant PR as Pull Request
    participant Jenkins as Jenkins CI
    participant Docker as Docker Container
    participant GenDocs as generate_docs.py
    participant GenMap as generate_mappings.py
    participant Schema as xlio_config_schema.json
    participant README as README

    PR->>Jenkins: Trigger CI pipeline
    Jenkins->>Jenkins: Check do_documentation_test flag
    alt do_documentation_test == true
        Jenkins->>Docker: Start documentation_test container (ubuntu22.04:build)
        Docker->>GenDocs: Execute python3 generate_docs.py
        GenDocs->>Schema: Read configuration schema
        Schema-->>GenDocs: Return schema data
        GenDocs->>GenDocs: Parse properties and generate docs
        GenDocs->>README: Update Configuration Values section
        README-->>GenDocs: Success/Failure
        Docker->>GenMap: Execute python3 generate_mappings.py
        GenMap->>Schema: Read configuration schema
        Schema-->>GenMap: Return schema data
        GenMap->>GenMap: Extract env var mappings
        GenMap->>GenMap: Generate mappings.py
        GenMap-->>Docker: Success/Failure
        Docker-->>Jenkins: Test result
        Jenkins-->>PR: Report documentation test status
    else do_documentation_test == false
        Jenkins->>Jenkins: Skip documentation test
    end
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@galnoam
Copy link
Collaborator

galnoam commented Oct 30, 2025

@MrBr-github, waiting for CI

@galnoam galnoam merged commit e398cd3 into Mellanox:vNext Oct 30, 2025
1 check passed
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.

5 participants