-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Migrate UI linting from pre-commit to GitHub Actions and add Makefile targets #24991
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
base: main
Are you sure you want to change the base?
Conversation
…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>
| pull_request_target: | ||
| types: [opened, synchronize, reopened, labeled] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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/**.
| branches: | ||
| - main | ||
| - "0.[0-9]+.[0-9]+" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
@copilot fix comments |
… .nvmrc, and use working-directory Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
|
@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. |
Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
|
Addressed in ac0c1be. Added The workflow already follows GitHub Actions security best practices for
|
|
@copilot fix suggestion and security fix from this comment: #24991 (comment) |
…ance Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.com>
Fixed in 03071d9. Updated |
|
@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>
Done in 6559364. Removed all husky and lint-staged setup from the UI directory:
UI linting is now completely handled by GitHub Actions workflow instead of local pre-commit hooks. |
🔍 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.Issuemaven-sonarcloud-ci code quality analysis job failed - another backend system failure. Root Cause AssessmentThis failure is UNRELATED to the PR changes. SonarCloud analyzes Java backend code quality. This PR contains:
Comprehensive CI Failure SummaryComplete list of CI failures for this PR: ✅ RELATED to PR (1 failure - requires fix):
❌ UNRELATED to PR (6+ failures - infrastructure/environmental): AnalysisPattern 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
Evidence:
Impact AssessmentFor PR Evaluation: This PR should be judged on:
NOT on:
These appear to be systemic CI issues requiring maintainer investigation. Code Review ✅ ApprovedClean 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 resolvedBug: Checkstyle script duplicates checks vs CI instructions comment
Code Quality: Redundant git diff-files check after read-only lint steps
Code Quality: Import check does not use working-directory consistently
Bug: YAML indentation errors will cause workflow parsing failure
Security: Using unpinned action reference from `main` branch
...and 2 more from earlier reviews What Works WellMoving 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 OptionsAuto-apply is off Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs) |
|
|



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: Removedlint-stagedhook 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 mirroringjava-checkstyle.ymlstructure. 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:yarn run lint)npx prettier --check)yarn run license-header-check)organize-imports-cli --check)Uses
.nvmrcfor Node version management (v22.17.0), yarn caching for improved performance, andworking-directoryparameter 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. Updatedpeter-evans/create-or-update-commentto v4 (from deprecated v1) for security compliance and improved functionality. Posts PR comment withmake lint-fixinstructions on failure.Makefile: Added UI linting targets:ui-lint/lint: Run all checks without modificationsui-lint-fix/lint-fix: Auto-fix all issuesRemoved UI pre-commit setup: Completely removed husky and lint-staged from UI directory:
.huskydirectory containing pre-commit hooks.lintstagedrc.yamlconfiguration filepre-commitandpreparescripts from package.jsonhuskyandlint-stageddependencies from package.jsonUsage:
Benefits:
pull_request_targetworkflowsSecurity:
pull_request_targetsafely with "safe to test" label verificationcontents: readandpull-requests: write)github.event.pull_request.head.shagithub.run_idfallbackpeter-evans/create-or-update-comment@v4)Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Original prompt
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.