Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 25, 2025

Describe your changes:

Completely decouples UI linting from pre-commit hooks to improve developer workflow. Pre-commit now only handles Java and Python files, while UI checks run exclusively in CI via GitHub Actions. All local UI pre-commit setup (husky and lint-staged) has been removed.

Changes:

  • .pre-commit-config.yaml: Removed lint-staged hook for TypeScript/JavaScript files. Retained Python hooks (pycln, isort, black), Java hook (google-style-java), and prettier for JSON schemas.

  • .github/workflows/eslint-prettier.yml: New workflow mirroring java-checkstyle.yml structure. Runs only on pull requests (not on direct pushes to main) with path filtering for UI directory changes (openmetadata-ui/src/main/resources/ui/**). Executes four parallel checks:

    • ESLint (yarn run lint)
    • Prettier (npx prettier --check)
    • License headers (yarn run license-header-check)
    • Import organization (organize-imports-cli --check)

    Uses .nvmrc for Node version management (v22.17.0), yarn caching for improved performance, and working-directory parameter for cleaner step definitions. Implements security best practices with concurrency group fallback (github.head_ref || github.run_id) to prevent issues with special characters in branch names. Updated peter-evans/create-or-update-comment to v4 (from deprecated v1) for security compliance and improved functionality. Posts PR comment with make lint-fix instructions on failure.

  • Makefile: Added UI linting targets:

    • ui-lint / lint: Run all checks without modifications
    • ui-lint-fix / lint-fix: Auto-fix all issues
  • Removed UI pre-commit setup: Completely removed husky and lint-staged from UI directory:

    • Deleted .husky directory containing pre-commit hooks
    • Deleted .lintstagedrc.yaml configuration file
    • Removed pre-commit and prepare scripts from package.json
    • Removed husky and lint-staged dependencies from package.json

Usage:

# Check UI code locally
make lint

# Fix all UI issues before commit
make lint-fix

Benefits:

  • No local git hooks for UI - cleaner developer setup
  • Faster pre-commit hooks (no UI processing at all)
  • Efficient CI enforcement - only runs on UI file changes, not on main branch pushes
  • Node version automatically synced with project's .nvmrc
  • Yarn caching for faster workflow execution
  • Clean workflow definition using working-directory parameter
  • Single command to fix all linting issues
  • Clear feedback in PR comments
  • Follows GitHub Actions security best practices for pull_request_target workflows

Security:

  • Uses pull_request_target safely with "safe to test" label verification
  • Minimal permissions (contents: read and pull-requests: write)
  • Explicit checkout reference using github.event.pull_request.head.sha
  • Concurrency group with github.run_id fallback
  • Updated to latest stable action versions (peter-evans/create-or-update-comment@v4)
  • No user-controlled input in run commands

Type of change:

  • Improvement

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • I have added tests around the new logic.
  • For connector/ingestion changes: I updated the documentation.
Original prompt

Remove UI-related pre-commit hooks from the .pre-commit-config.yaml file and ensure it aligns with the team's requirements for Java files and Python files only.

Add a GitHub Actions workflow named eslint-prettier.yml that checks for eslint, prettier, license headers, and import checks on each push or pull request targeting the main branch. Reference the structure and logic from the existing java-checkstyle.yml workflow.

Modify the Makefile to include make commands for lint and lint-fix. The lint command should execute eslint, prettier, license, and import checks, while lint-fix should auto-fix these issues where possible. Specifically, the commands npm run lint, npm run prettier:check, npm run license-check, and their fixing equivalents should be featured in the respective rules.
Ensure that all changes maintain consistency with the existing repository structure and provide a seamless developer experience.

This pull request was created from Copilot chat.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…ith Makefile targets

Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove UI pre-commit hooks and add linting commands Migrate UI linting from pre-commit to GitHub Actions and add Makefile targets Dec 25, 2025
Copilot AI requested a review from chirag-madlani December 25, 2025 06:02
Comment on lines 21 to 22
pull_request_target:
types: [opened, synchronize, reopened, labeled]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please limit this workflow for openmetadata-ui/src/main/resources/ui file changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a092c58. Added paths filter to only trigger workflow for changes in openmetadata-ui/src/main/resources/ui/**.

Comment on lines 18 to 20
branches:
- main
- "0.[0-9]+.[0-9]+"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't wanna run it on main commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a092c58. Removed push trigger - workflow now only runs on pull_request_target events.

- name: Set up Node.js
uses: actions/setup-node@v4
with:
node-version: '18'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use nvmrc to determine node version instead hardcoded one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a092c58. Now using node-version-file: 'openmetadata-ui/src/main/resources/ui/.nvmrc' and added yarn cache configuration.

id: imports
continue-on-error: true
run: |
cd openmetadata-ui/src/main/resources/ui
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we update path via working-directory instead doing cd on each step

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a092c58. All steps now use working-directory: openmetadata-ui/src/main/resources/ui parameter instead of cd commands.

@chirag-madlani
Copy link
Collaborator

@copilot fix comments

… .nvmrc, and use working-directory

Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
@chirag-madlani
Copy link
Collaborator

@copilot Please @github-advance-security comments and @gitar-bot comments

Copy link
Contributor Author

Copilot AI commented Dec 25, 2025

@copilot Please @github-advance-security comments and @gitar-bot comments

I don't see any comments from @github-advance-security or @gitar-bot in the current PR context. Could you please point me to the specific comments or share what issues they've flagged? I'd be happy to address them once I can see the feedback.

@chirag-madlani
Copy link
Collaborator

@copilot address this comment security and suggestions

Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 25, 2025

@copilot address this comment security and suggestions

Addressed in ac0c1be. Added github.run_id fallback to the concurrency group following security best practices used in other workflows in the repository. This prevents potential issues with special characters in branch names.

The workflow already follows GitHub Actions security best practices for pull_request_target:

  • Requires "safe to test" label verification before running
  • Minimal permissions (contents: read + pull-requests: write)
  • Explicit checkout reference using github.event.pull_request.head.sha
  • No user-controlled input in run commands

@chirag-madlani
Copy link
Collaborator

@copilot fix suggestion and security fix from this comment: #24991 (comment)

…ance

Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 27, 2025

@copilot fix suggestion and security fix from this comment: #24991 (comment)

Fixed in 03071d9. Updated peter-evans/create-or-update-comment from @v1 to @v4 to address the security recommendation. The v1 version is deprecated and v4 is the current stable version with security improvements and bug fixes.

@chirag-madlani
Copy link
Collaborator

@copilot Remove husky and lint-staged setup from openmetadata-ui/src/main/resource/ui directory to remove pre-commit completely

Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 27, 2025

@copilot Remove husky and lint-staged setup from openmetadata-ui/src/main/resource/ui directory to remove pre-commit completely

Done in 6559364. Removed all husky and lint-staged setup from the UI directory:

  • Deleted .husky directory with pre-commit hooks
  • Deleted .lintstagedrc.yaml configuration file
  • Removed pre-commit and prepare scripts from package.json
  • Removed husky and lint-staged dependencies from package.json

UI linting is now completely handled by GitHub Actions workflow instead of local pre-commit hooks.

@gitar-bot
Copy link

gitar-bot bot commented Jan 2, 2026

🔍 CI failure analysis for 4fa21ae: maven-sonarcloud-ci backend code analysis failure is unrelated to PR which only modifies UI linting infrastructure - part of widespread CI infrastructure issues affecting this PR.

Issue

maven-sonarcloud-ci code quality analysis job failed - another backend system failure.

Root Cause Assessment

This failure is UNRELATED to the PR changes.

SonarCloud analyzes Java backend code quality. This PR contains:

  • ❌ Zero Java code modifications
  • ❌ Zero backend code changes
  • ✅ Only UI linting infrastructure changes
  • ✅ Only frontend code formatting changes

Comprehensive CI Failure Summary

Complete list of CI failures for this PR:

RELATED to PR (1 failure - requires fix):

  1. ui-checkstyle: Dependency installation failures (tree-sitter/antlr4)
    • Status: Directly caused by PR's new workflow
    • Action needed: Install antlr4 in CI or update dependencies

UNRELATED to PR (6+ failures - infrastructure/environmental):
2. playwright-ci-postgresql (6, 6): Test environment setup
3. playwright-ci-postgresql (2, 6): E2E test execution
4. py-run-tests (3.11): Python backend tests
5. maven-postgresql-ci: Java backend tests
6. maven-sonarcloud-ci: Java code quality analysis

Analysis

Pattern Recognition:

With 6 different test systems failing (UI deps, Playwright E2E, Python tests, Java tests, Java code analysis) across completely independent codebases, while the PR only touches UI linting infrastructure, this clearly indicates:

NOT a code problem from this PR
Likely causes:

  • CI infrastructure instability
  • Resource constraints
  • Environmental issues
  • Pre-existing test flakiness
  • Timing/network issues

Evidence:

  • PR modifies only UI linting (workflow, pre-commit, Makefile, formatting)
  • No functional logic changes
  • No backend code touched
  • Multiple independent systems failing simultaneously

Impact Assessment

For PR Evaluation:

This PR should be judged on:

  1. ✅ Linting workflow design (good)
  2. ❌ ui-checkstyle execution (needs dependency fix)
  3. ✅ Code formatting quality (good)
  4. ✅ Infrastructure cleanup (removed husky/lint-staged)

NOT on:

  • Unrelated Playwright failures
  • Unrelated Python test failures
  • Unrelated Java test failures
  • Unrelated SonarCloud analysis

These appear to be systemic CI issues requiring maintainer investigation.

Code Review ✅ Approved

Clean infrastructure refactor moving UI linting from local pre-commit hooks to CI workflow. All changes are formatting-only with no functional logic modifications.

Resolved ✅ 7 resolved
Bug: Checkstyle script duplicates checks vs CI instructions comment

📄 .github/workflows/ui-checkstyle.yml:84-88
The workflow runs yarn run checkstyle which executes:

yarn run lint && yarn run pretty:check && yarn run license-header-check && npx organize-imports-cli --check 'src/**/*.{ts,tsx,js,jsx}'

However, the PR comment instructions (lines 98-104) tell users to run different commands:

yarn run lint:fix
yarn run pretty
yarn run license-header-fix
npx organize-imports-cli 'src/**/*.{ts,tsx,js,jsx}'

The comment correctly uses the fix variants. However, this mismatch could be confusing. Consider adding a note that yarn run checkstyle runs the check variants while the fix commands auto-fix issues.

Code Quality: Redundant git diff-files check after read-only lint steps

📄 .github/workflows/eslint-prettier.yml:88-92
The git diff-files --quiet check at line 91-92 is running after steps that only check (not fix) code. Since yarn run lint, npx prettier --check, yarn run license-header-check, and npx organize-imports-cli --check don't modify files, this git check will never detect changes from these steps.

This step appears to be copied from a pattern where auto-fix steps were run, but in this read-only CI workflow, it serves no purpose.

Recommendation: Either remove this step entirely, or if it's meant to catch uncommitted changes that existed before the workflow ran, add a comment explaining its purpose.

Code Quality: Import check does not use working-directory consistently

📄 .github/workflows/eslint-prettier.yml:83-86
The Check import organization step at line 83-86 runs organize-imports-cli with a relative path pattern 'src/**/*.{ts,tsx,js,jsx}'. This relies on the working-directory being set correctly, which it is. However, the pattern will be interpreted by the shell before organize-imports-cli sees it.

On CI runners, the shell may expand the glob differently depending on shell settings. Consider wrapping the pattern in double quotes or using yarn run with a package.json script for consistency with other steps.

Suggested fix:

run: npx organize-imports-cli --check "src/**/*.{ts,tsx,js,jsx}"

Or add an npm script and use yarn run import-check.

Bug: YAML indentation errors will cause workflow parsing failure

📄 .github/workflows/eslint-prettier.yml:39
The workflow file has critical YAML indentation issues that will cause GitHub Actions to fail parsing:

  1. Line 39: steps: is incorrectly indented under permissions: block - it should be a direct child of the job, not nested under permissions
  2. Lines 55-59: The Set up Node.js step has incorrect indentation (uses 8 spaces instead of 10)
  3. Lines 61-88+: Remaining steps use inconsistent 6-space indentation instead of 10 spaces

The correct structure should have:

  • steps: at 4 spaces (same level as permissions: within the job)
  • Each step at 6 spaces with - prefix
  • Step content at 8 spaces

Suggested fix: Restructure the YAML so steps: is a sibling of permissions: under the job, and use consistent 2-space indentation throughout:

jobs:
  eslint-prettier:
    runs-on: ubuntu-latest
    permissions:
      pull-requests: write
    steps:
      - name: Free Disk Space (Ubuntu)
        uses: jlumbroso/free-disk-space@v1.3.1
        ...
Security: Using unpinned action reference from `main` branch

📄 .github/workflows/eslint-prettier.yml:41 🔗 GitHub Actions Security Best Practices
The workflow uses jlumbroso/free-disk-space@main which references the main branch instead of a pinned version or SHA. This creates a supply chain security risk where changes to the upstream action could unexpectedly affect your workflow or potentially introduce malicious code.

Impact: If the upstream repository is compromised or makes breaking changes, your CI workflow could be affected without warning.

Suggested fix: Pin to a specific release version or commit SHA:

uses: jlumbroso/free-disk-space@v1.3.1

or with SHA for maximum security:

uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be  # v1.3.1

...and 2 more from earlier reviews

What Works Well

Moving linting to CI workflow is a good practice - it ensures consistent enforcement across all contributors regardless of local setup. The Makefile targets provide convenient local linting commands for developers. Removing unused dependencies (husky, lint-staged) reduces the dependency footprint.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off Gitar will not commit updates to this branch.
✅ Code review is on Gitar will review this change.
Display: compact Hiding non-applicable rules.

Comment with these commands to change:

Auto-apply ✅ Code review Compact
gitar auto-apply:on         
gitar code-review:off         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.77% (51490/79499) 42.47% (25173/59279) 45.97% (7949/17293)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 2, 2026

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 2, 2026

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

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants