Skip to content
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.

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

[DRAFT] Add pre-commit to start standardizing commits and file formats #1573

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jvoravong
Copy link
Contributor

@jvoravong jvoravong commented Mar 14, 2025

Description:
This repository could benefit from using pre-commit to help standardize formatting and prevent long-running formatting issues.

While working on a work ticket, I encountered issues caused by inconsistent indentation (e.g., a mix of spaces and tabs) in the values.schema.json file for the operator chart. Inconsistent formatting can cause linting failures and prevent deploying the chart to certain Kubernetes environments.

Changes in this PR:

  • Added pre-commit configuration files so that pre-commit runs automatically when developers commit locally.
  • Added a GitHub workflow to run pre-commit on every PR commit and merge into main.
  • Updated CONTRIBUTING.md to document pre-commit as a developer dependency.
  • Introduced initial pre-commit hooks:
    • Large file check
    • JSON format validation
    • Pretty-format JSON ✅ (needed to reformat a few files for this to pass)
  • Standardized and fixed JSON formatting issues in multiple files, including:
    • charts/opentelemetry-operator/values.schema.json
    • charts/opentelemetry-demo/grafana-dashboards/*.json
    • charts/opentelemetry-demo/products/products.json
    • charts/opentelemetry-ebpf/values.schema.json
    • charts/opentelemetry-kube-stack/values.schema.json

Chart Version Bump?

  • I'm unsure if the values.schema.json changes require a version bump since they don't affect functionality or templates. Should I bump the version each chart affected? I guess I would do this in separate PRs one at a time.
  • However, the opentelemetry-demo project required minor Grafana dashboard formatting updates to pass pretty-format-json, which did necessitate a patch version bump. To keep changes minimal, I separated that into a different PR ([demo] Refactor json values so they can work with pre-commit json hooks #1572) which will have to be merged first.

Next Steps

  • There are several additional pre-commit hooks we should add in the future, but doing them all at once would make this PR too large.
  • I’ve included a TODO to track adding more recommended pre-commit hooks in a follow-up PRs.

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