Skip to content

fix: correct yaml linting hook and fix trailing spaces/comment spacing #611

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

Jump to bottom
Merged
merged 4 commits into from
Nov 7, 2025
Merged

fix: correct yaml linting hook and fix trailing spaces/comment spacing #611

merged 4 commits into from
Nov 7, 2025

Conversation

@yossiovadia
Copy link
Collaborator

@yossiovadia yossiovadia commented Nov 7, 2025

Summary

This PR addresses the confusion from PRs #609 and #610 by providing a complete solution that includes:

  1. Fixed pre-commit hook configuration bug (from PR fix: correct yaml linting hook to call yaml-lint instead of markdown-lint #609)
  2. Fixed simple YAML linting errors that were causing CI failures

Problem

The bug in .pre-commit-config.yaml line 57 caused:

  • ❌ YAML files not being properly linted locally
  • ✅ GitHub Actions CI catching the issues (because it properly installs yamllint)
  • 🤔 PRs failing in CI even though pre-commit run --all-files passes locally
  • 😓 Contributors forced to fix pre-existing YAML issues in files they never touched

Changes

1. Pre-commit Hook Fix

Changed line 57 in .pre-commit-config.yaml:

# Before
entry: bash -c "make markdown-lint"

# After
entry: bash -c "make yaml-lint"

2. YAML Formatting Fixes

Applied automated fixes for 22 YAML files:

  • ✅ Removed trailing whitespace
  • ✅ Fixed comment spacing (ensuring 2 spaces before inline comments)

Testing

After the fix:

$ pre-commit run yaml-and-yml-fmt --all-files
yaml/yml fmt.............................................................Failed
[Shows actual YAML linting errors that need manual fixing]

This confirms the hook now properly runs yamllint and catches issues that GitHub Actions CI catches.

Note on Remaining Issues

Some YAML files still have indentation errors that require more careful manual fixes. These are out of scope for this PR and can be addressed in follow-up PRs as files are modified. The important achievement here is that local pre-commit checks now match what GitHub Actions CI checks.

Relationship to Previous PRs

Fixes #608

Checklist

  • Fixed the pre-commit hook to call correct command
  • Applied automated YAML formatting fixes (trailing spaces, comment spacing)
  • Tested locally that the hook now catches YAML linting errors
  • Commit includes DCO sign-off
  • References the issue number

@yossiovadia @claude
fix: correct yaml linting hook and fix trailing spaces/comment spacing
019b87b 
This PR addresses two issues:

1. **Fixed pre-commit hook configuration bug** - Changed line 57 in
   `.pre-commit-config.yaml` to call `make yaml-lint` instead of
   `make markdown-lint`

2. **Fixed simple YAML linting errors** - Applied automated fixes for:
   - Trailing whitespace in YAML files
   - Comment spacing (ensuring 2 spaces before inline comments)

## Problem

The bug in `.pre-commit-config.yaml` caused:
- ❌ YAML files not being properly linted locally
- ✅ GitHub Actions CI catching the issues
- 🤔 PRs failing in CI even though `pre-commit run --all-files` passed locally
- 😓 Contributors forced to fix pre-existing YAML issues

## Changes

1. Changed `.pre-commit-config.yaml` line 57 from `make markdown-lint` to `make yaml-lint`
2. Fixed trailing spaces and comment spacing in 22 YAML files

## Note on Remaining Issues

Some YAML files still have indentation errors that require more careful
manual fixes. These can be addressed in follow-up PRs as files are modified.
The important fix here is that local pre-commit checks now match CI checks.

Fixes vllm-project#608

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Yossi Ovadia <[email protected]>
@netlify
Copy link

netlify bot commented Nov 7, 2025 •
edited
Loading

✅ Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit f2f57ed
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/690e42bdbd91850008b7592c
😎 Deploy Preview https://deploy-preview-611--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions
Copy link

github-actions bot commented Nov 7, 2025 •
edited
Loading

👥 vLLM Semantic Team Notification

The following members have been identified for the changed files in this PR and have been automatically assigned:

📁 Root Directory

Owners: @rootfs, @Xunzhuo
Files changed:

  • .pre-commit-config.yaml
  • examples/semanticroute/comprehensive-example.yaml
  • examples/semanticroute/multiple-routes.yaml

📁 config

Owners: @rootfs, @Xunzhuo
Files changed:

  • config/config.yaml
  • config/intelligent-routing/in-tree/bert_classification.yaml
  • config/intelligent-routing/in-tree/keyword.yaml
  • config/intelligent-routing/out-tree/config-mcp-classifier.yaml
  • config/observability/config.tracing.yaml
  • config/prompt-guard/pii_domain.yaml
  • config/semantic-cache/config.hybrid.yaml

📁 deploy

Owners: @rootfs, @Xunzhuo
Files changed:

  • deploy/docker-compose/docker-compose.yml
  • deploy/kubernetes/ai-gateway/aigw-resources/base-model.yaml
  • deploy/kubernetes/ai-gateway/aigw-resources/gwapi-resources.yaml
  • deploy/kubernetes/ai-gateway/semantic-router/config.yaml
  • deploy/kubernetes/ai-gateway/semantic-router/deployment.yaml
  • deploy/kubernetes/aibrix/aigw-resources/base-model.yaml
  • deploy/kubernetes/aibrix/aigw-resources/gwapi-resources.yaml
  • deploy/kubernetes/aibrix/semantic-router/config.yaml
  • deploy/kubernetes/aibrix/semantic-router/deployment.yaml
  • deploy/kubernetes/istio/config.yaml
  • deploy/kubernetes/istio/deployment.yaml
  • deploy/kubernetes/istio/destinationrule.yaml
  • deploy/kubernetes/istio/envoyfilter.yaml
  • deploy/kubernetes/istio/httproute-llama3-8b.yaml
  • deploy/kubernetes/istio/httproute-phi4-mini.yaml
  • deploy/kubernetes/istio/kustomization.yaml
  • deploy/kubernetes/istio/vLlama3.yaml
  • deploy/kubernetes/istio/vPhi4.yaml
  • deploy/kubernetes/llmd-base/dest-rule-epp-llama.yaml
  • deploy/kubernetes/llmd-base/dest-rule-epp-phi4.yaml
  • deploy/kubernetes/llmd-base/httproute-llama-pool.yaml
  • deploy/kubernetes/llmd-base/httproute-phi4-pool.yaml
  • deploy/kubernetes/llmd-base/inferencepool-llama.yaml
  • deploy/kubernetes/llmd-base/inferencepool-phi4.yaml
  • deploy/kubernetes/observability/dashboard/config.yaml
  • deploy/openshift/observability/prometheus/deployment.yaml
  • deploy/openshift/template.yaml

📁 tools

Owners: @yuluo-yx, @rootfs, @Xunzhuo
Files changed:

  • tools/linter/go/.golangci.yml
  • tools/linter/yaml/.yamllint
vLLM

🎉 Thanks for your contributions!

This comment was automatically generated based on the OWNER files in the repository.

@yossiovadia
Copy link
Collaborator Author

yossiovadia commented Nov 7, 2025

looking at the failures

@yossiovadia @claude
fix: resolve YAML indentation errors and exclude .venv from linting
c45973e 
This commit fixes the pre-commit failures from the previous commit by:

1. **Fixed YAML indentation errors** in 16 Kubernetes/OpenShift deployment files
   - Re-parsed and reformatted YAML files with proper 2-space indentation
   - Fixed wrong indentation issues flagged by yamllint

2. **Excluded .venv from yamllint checks**
   - Added `.venv` to the ignore list in `tools/linter/yaml/.yamllint`
   - Prevents linting errors from third-party dependencies in virtual environment

Files fixed:
- deploy/kubernetes/ai-gateway/aigw-resources/*
- deploy/kubernetes/aibrix/aigw-resources/*
- deploy/kubernetes/istio/*
- deploy/kubernetes/llmd-base/*
- deploy/openshift/observability/prometheus/deployment.yaml
- deploy/openshift/template.yaml

Pre-commit now passes successfully.

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Yossi Ovadia <[email protected]>
@yossiovadia yossiovadia force-pushed the fix/pre-commit-yaml-lint-with-fixes branch from 028c938 to c45973e Compare November 7, 2025 18:33
@yossiovadia
Copy link
Collaborator Author

yossiovadia commented Nov 7, 2025

this failed test, test has not been executed for a while now.

it seems that chnage of:
config/config.yaml
deploy/docker-compose/docker-compose.yml

triggers quickstart test, which seems to fail on
'GitHub Actions doesn't have credentials to download the gated model google/embeddinggemma-300m from HuggingFace'
which is in not way related to this PR.

@rootfs rootfs merged commit dd391fd into vllm-project:main Nov 7, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rootfs rootfs rootfs approved these changes

@Xunzhuo Xunzhuo Awaiting requested review from Xunzhuo Xunzhuo is a code owner

Assignees

@rootfs rootfs

@Xunzhuo Xunzhuo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix: pre-commit yaml linting hook calls wrong command (markdown-lint instead of yaml-lint)

3 participants

@yossiovadia @rootfs @Xunzhuo