HDDS-14770. [Website] Integrate eslint and run it in github actions#359
HDDS-14770. [Website] Integrate eslint and run it in github actions#359yuriipalam wants to merge 2 commits intoapache:masterfrom
Conversation
|
Thanks a lot for this much needed change @yuriipalam. I am not sure if this is being currently used or if it would be used in the future, but enforcing this would be great. |
|
@devabhishekpal thanks for the review. For this I'm planning to add Prettier in the next PR. It enforces all the code formatting rules, so they are consistent. What you said could be also enforced by eslint alone, but since I'm planning to add Prettier in the subsequent PR, it's better to do it there. I'll enforce exactly what you've requested. |
devabhishekpal
left a comment
There was a problem hiding this comment.
Thanks for the patch @yuriipalam.
This LGTM, +1
errose28
left a comment
There was a problem hiding this comment.
Thanks for adding this @yuriipalam
| "lint": "eslint . && markdownlint \"$(git rev-parse --show-toplevel)\" && yamllint --format=colored \"$(git rev-parse --show-toplevel)\"", | ||
| "lint:fix": "eslint --fix . && markdownlint --fix \"$(git rev-parse --show-toplevel)\" && yamllint --format=colored \"$(git rev-parse --show-toplevel)\"" |
There was a problem hiding this comment.
After some testing, it looks like the . is always resolved to the repo root regardless of which tool is used. Also switching && for ; lets us run all linters at once instead of stopping when the first one hits an error.
| "lint": "eslint . && markdownlint \"$(git rev-parse --show-toplevel)\" && yamllint --format=colored \"$(git rev-parse --show-toplevel)\"", | |
| "lint:fix": "eslint --fix . && markdownlint --fix \"$(git rev-parse --show-toplevel)\" && yamllint --format=colored \"$(git rev-parse --show-toplevel)\"" | |
| "lint": "eslint . ; markdownlint . ; yamllint --format=colored .", | |
| "lint:fix": "eslint --fix . ; markdownlint --fix . ; yamllint --format=colored ." |
To test this I added this diff and ran pnpm lint from the docs directory, which is lower in the directory tree than any of these changes. Linting errors on all three files were correctly flagged.
diff --git a/compose.yml b/compose.yml
index 1a6c84659..32e67770b 100644
--- a/compose.yml
+++ b/compose.yml
@@ -17,7 +17,7 @@ version: "3"
services:
site:
- build: .
+ build: .
image: ozone-site
ports:
- 3001:3001
diff --git a/package.json b/package.json
index 4277cbaf5..e0739a14b 100644
--- a/package.json
+++ b/package.json
@@ -13,8 +13,8 @@
"serve": "docusaurus serve --port 3001",
"write-translations": "docusaurus write-translations",
"write-heading-ids": "docusaurus write-heading-ids",
- "lint": "eslint . && markdownlint \"$(git rev-parse --show-toplevel)\" && yamllint --format=colored \"$(git rev-parse --show-toplevel)\"",
- "lint:fix": "eslint --fix . && markdownlint --fix \"$(git rev-parse --show-toplevel)\" && yamllint --format=colored \"$(git rev-parse --show-toplevel)\""
+ "lint": "eslint . ; markdownlint . ; yamllint --format=colored .",
+ "lint:fix": "eslint --fix . ; markdownlint --fix . ; yamllint --format=colored ."
},
"dependencies": {
"@docusaurus/core": "3.7.0",
diff --git a/src/pages/download.md b/src/pages/download.md
index 2fefd7658..c543ef39e 100644
--- a/src/pages/download.md
+++ b/src/pages/download.md
@@ -6,7 +6,7 @@
| 2.0.0 | 2025 Apr 30 | [Source](https://www.apache.org/dyn/closer.cgi/ozone/2.0.0/ozone-2.0.0-src.tar.gz)<br/>[Checksum](https://downloads.apache.org/ozone/2.0.0/ozone-2.0.0-src.tar.gz.sha512)<br/>[Signature](https://downloads.apache.org/ozone/2.0.0/ozone-2.0.0-src.tar.gz.asc) | [Binary](https://www.apache.org/dyn/closer.cgi/ozone/2.0.0/ozone-2.0.0.tar.gz)<br/>[Checksum](https://downloads.apache.org/ozone/2.0.0/ozone-2.0.0.tar.gz.sha512)<br/>[Signature](https://downloads.apache.org/ozone/2.0.0/ozone-2.0.0.tar.gz.asc) | [Release Notes](/release-notes/2.0.0) |
| 1.4.1 | 2024 Nov 24 | [Source](https://www.apache.org/dyn/closer.cgi/ozone/1.4.1/ozone-1.4.1-src.tar.gz)<br/>[Checksum](https://downloads.apache.org/ozone/1.4.1/ozone-1.4.1-src.tar.gz.sha512)<br/>[Signature](https://downloads.apache.org/ozone/1.4.1/ozone-1.4.1-src.tar.gz.asc) | [Binary](https://www.apache.org/dyn/closer.cgi/ozone/1.4.1/ozone-1.4.1.tar.gz)<br/>[Checksum](https://downloads.apache.org/ozone/1.4.1/ozone-1.4.1.tar.gz.sha512)<br/>[Signature](https://downloads.apache.org/ozone/1.4.1/ozone-1.4.1.tar.gz.asc) | [Release Notes](/release-notes/1.4.1) |
-## Archives
+### Archives
Older releases are available from the [Apache Ozone archive](https://archive.apache.org/dist/ozone/).
diff --git a/src/theme/DocSidebar/index.module.css b/src/theme/DocSidebar/index.module.css
index 39601f08f..c3d7447af 100644
--- a/src/theme/DocSidebar/index.module.css
+++ b/src/theme/DocSidebar/index.module.css
@@ -48,6 +48,6 @@ aside .customSidebarInner {
aside .customSidebarVersion p {
margin-bottom: 0;
- font-size: var(--fbc-font-size, 13px);
+ font-size: var(--fbc-font-size: 13px;);
padding: var(--ifm-menu-link-padding-vertical) var(--ifm-menu-link-padding-horizontal);
}| "static/**", | ||
| "node_modules/**", | ||
| "build/**", | ||
| "dist/**", |
There was a problem hiding this comment.
I don't see a dist directory in the repo. Does something generate this?
| "unused-imports/no-unused-imports": "error", | ||
| // unused vars (but ignore underscore-prefixed) | ||
| "unused-imports/no-unused-vars": [ | ||
| "warn", |
There was a problem hiding this comment.
I don't think warning are useful, people will only check if the command passes or fails. If we care about something we should make it an error. Unused variables should fail linting I think.
| // Custom rule to enforce Apache License header | ||
| { | ||
| files: ["**/*.{js,mjs,cjs,jsx}"], |
There was a problem hiding this comment.
I don't think we need this. We already have a license header check using skywalking-eyes for all files in CI which is more robust than this strict string constant header check. This does run locally on pnpm lint which is nice, but it only checks javascript files which are rarely modified. I don't think we would want to expand the scope to other file types since it's more brittle than skywalking eyes anyways.
I think we can remove this to keep our linting config simple and reduce duplication/conflicts with skywalking-eyes. In a follow-up change it would be good to have skywalking-eyes run locally on lint too, but that would also require a separate install outside of node.js.
| "unused-imports": pluginUnusedImports, | ||
| import: pluginImport, | ||
| }, | ||
| rules: { |
There was a problem hiding this comment.
Do we want to add a rule to enforce import order?
| "@docusaurus/prefer-docusaurus-heading": "error", | ||
| "@docusaurus/string-literal-i18n-messages": "error", | ||
| // for i18n | ||
| // "@docusaurus/no-untranslated-text": ["warn", { ignoredStrings: [] }] |
There was a problem hiding this comment.
Does this rule fail even if i18n is disabled? If it passes trivially in this case then we can probably reduce this to just use the default recommended config instead of specifying rules individually.
| { | ||
| files: ["**/*.{js,mjs,cjs,jsx}"], | ||
| ...js.configs.recommended, | ||
| languageOptions: { globals: { ...globals.browser, ...globals.node } }, |
There was a problem hiding this comment.
Cursor made this suggestion, although I don't know enough about javascript to really understand the implications:
Medium: browser and node globals are both enabled for all JS files, which can hide environment mistakes.
This permits Node globals in browser code and browser globals in Node-only files without lint failures.
Better practice is split overrides by file location (src/** browser, config/scripts Node).
files: ["**/*.{js,mjs,cjs,jsx}"],
...js.configs.recommended,
languageOptions: { globals: { ...globals.browser, ...globals.node } },
plugins: {
"unused-imports": pluginUnusedImports,
import: pluginImport,
},
files: ["**/*.{js,mjs,cjs,jsx}"],...js.configs.recommended,languageOptions: { globals: { ...globals.browser, ...globals.node } },plugins: { "unused-imports": pluginUnusedImports, import: pluginImport,},
What changes were proposed in this pull request?
Integrate ESLint into the project.
ESLint is a static code analyzer, so now we have analyzer during the development.
Both tools were integrated, as well as relevant extensions to them, e.g., Docusaurus ESLint plugin
Fixed all the issues that the code analyzers found. Some files had
.jsextension, while they are supposed to have.jsxextension, since they have React code inside.pnpm run lintnow runseslintas well.pnpm run lint:fixnow applies auto-fixes from ESLint as well.Strongly recommend you to check
eslint.config.mjs, since it sets up the code rules for the project. I also added a custom rule that enforces the license header.I also updated
CONTRIBUTE.MDaccordingly, the "Quick Start" section and added a new "Linting and Formatting" section.What is the link to the Apache Jira?
HDDS-14770
How was this patch tested?
Check off which of the following tests were done on this change. If additional testing was done, please elaborate here as well.