[FLINK-23113][runtime-web] Migrate to ESLint, add Stylint and address dependency vulnerabilities #16902

yangjunhan · 2021-08-20T02:35:20Z

What is the purpose of the change

TSLint is no longer maintained, so runtime-web should be migrating to ESLint for Angular. In addition, stylelint should be added for .less stylesheets which is originally missing.

Brief change log

Migrate to ESLint
Add Stylelint
--fix all .{less,html,ts} file according to the new rules and prettier config
Address dependency vulnerabilities
Enforce linting in CI

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? NA

flinkbot · 2021-08-20T02:38:43Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit af560fe (Fri Aug 20 02:38:43 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!
This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2021-08-20T03:02:07Z

CI report:

5240b15 UNKNOWN
bc16f1e UNKNOWN
6d6d751 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

yangjunhan · 2021-08-20T03:41:44Z

Please note, this PR also covers "Address vulnerabilities in Flink UI" FLINK-23114.

Airblader

Thanks @yangjunhan for the PR! It'll be a great improvement to Flink UI to have proper linting set up. I will take a closer look next week.

However, it would be good if we could split this into different commits, in particular actually applying all linting fixes should be its own commit. This would make the review easier, allow us to regenerate that commit upon conflicts and maybe even add it to .git-blame-ignore-revs (during merging). Since you mentioned the vulnerability fixes here, anything not related to tslint/eslint itself should also be a separate commit as those changes are important and useful on their own.

Another note: this currently isn't checked in the CI, correct? We need to make sure that the CI actually verifies using eslint as well (again, as a separate commit).

Airblader · 2021-08-20T05:24:25Z

flink-runtime-web/web-dashboard/src/app/interfaces/safe-any.ts

@@ -0,0 +1,2 @@
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export type SafeAny = any;


We should immediately mark this as deprecated. SafeAny is not safe at all, it's a hack that silences a correct warning/error. It would be better to use unknown instead everywhere, however that is a separate change as it isn't a drop-in replacement.

Airblader · 2021-08-20T05:25:51Z

flink-runtime-web/web-dashboard/.eslintignore

+.eslintcache
+
+# Yarn Integrity file
+.yarn-integrity


We don't use yarn, do we (also below for .yarn)?

yangjunhan · 2021-08-20T08:44:25Z

Thanks @yangjunhan for the PR! It'll be a great improvement to Flink UI to have proper linting set up. I will take a closer look next week.

However, it would be good if we could split this into different commits, in particular actually applying all linting fixes should be its own commit. This would make the review easier, allow us to regenerate that commit upon conflicts and maybe even add it to .git-blame-ignore-revs (during merging). Since you mentioned the vulnerability fixes here, anything not related to tslint/eslint itself should also be a separate commit as those changes are important and useful on their own.

Another note: this currently isn't checked in the CI, correct? We need to make sure that the CI actually verifies using eslint as well (again, as a separate commit).

Thank you for your advice @Airblader. Now I have split it into several individual commits and hopefully they make sense to you. Regarding the SafeAny issue, I already marked it as deprecated, and perhaps we will remove it in the near future.

As for the CI, can you give me some guide about the shell file that should be modified? I could not find it under ./tool/ci/.

Airblader · 2021-08-23T06:11:46Z

FYI, the CI currently fails because it doesn't know npx.

As for the CI, can you give me some guide about the shell file that should be modified? I could not find it under ./tool/ci/.

In flink-runtime-web/pom.xml, first npm ci and then npm run build is executed. I think we should hook into the build script to also run the linter first. An alternative would be to create a separate npm script which does lint+build and then call that from pom.xml.

Once the build for this PR is passing we should intentionally introduce a commit that breaks the linter to verify that the CI is picking up on it.

yangjunhan · 2021-08-25T02:21:17Z

Hi, @Airblader. I finish enforcing the Lint during the CI build and doing several changes to make the CI to pass. There are a few things that are worthwhile to pay attention to:

Frontend-maven-plugin was previously installed as v1.6 which was considered as outdated for a while. Especially there was a bug which ignored npm installation if npmVersion was not explicitly stated. So I take the liberty to update this plugin to a newer version v1.9.1 and explicitly specific the npm's version in the pom.xml.
Despite the fact of running npm audit fix, even with --force, there is still a moderate vulnerability regarding @angular-devkit/build-angular's dependency, glob-parent >=5.1.2. The reason I leave it there is due to this issue: #21214. Angular CLI team is planning to solve this by upgrading to webpack-dev-server 4 soon.

Airblader

Thanks for your work on this! I think this is looking pretty good now overall. We need to wait until after 1.14 with this, I think; while it's an non-functional change, it does update things like dependencies.

Airblader · 2021-08-25T05:54:52Z

flink-runtime-web/web-dashboard/.stylelintrc.js

+        severity: 'warning'
+      }
+    ],
+    'order/properties-order': [


In general I don't want to focus on the individual settings in this PR, we can always improve those later if we feel any of them are not ideal. However, for this one I'm just curious how this list was created. It's pretty long and seems quite opinionated? Do we need/want to be this strict with the order of properties?

Well, this piece of info is taken from: https://github.com/hudochenkov/stylelint-config-hudochenkov/blob/master/order.js. The author of this config is actually a member of Stylelint team. There is a comment about this at line 22.

After thinking over it, it may not be a good idea to extract the source code from this config. Instead, I add it as a dev dependency and extend it in the .stylelintrc.js file as the README suggests.

Airblader · 2021-08-25T05:57:41Z

flink-runtime-web/src/main/resources/META-INF/NOTICE

@@ -13,6 +13,11 @@ The Apache Software Foundation (http://www.apache.org/).
 @angular/platform-browser 12.1.3 : MIT License
 @angular/platform-browser-dynamic 12.1.3 : MIT License
 @angular/router 12.1.3 : MIT License
+@angular-eslint/builder 12.3.1 : MIT License


All of these are dev dependencies, right? Do we even need to add them to the NOTICE file? It seems at least any other dev dependency I looked at isn't mentioned here either. I'll see if I can find out whether this is necessary.

Okay, thanks. In fact, I am not quite sure about this either. I will remove them if they are unnecessary.

Since the final release doesn't contain these dependencies they should not need to be listed. I think we can remove this.

No problems 👌

Airblader

Now that the 1.14 release branch has been cut we can proceed here and get this merged. I think overall this looks fine now, however there is one crucial issue still with the npm scripts. Afterwards this should be good to merge!

Airblader · 2021-09-02T07:21:09Z

flink-runtime-web/web-dashboard/package.json

+    "lint": "eslint --cache src --ext .ts,.html & stylelint \"**/*.less\"",
+    "lint:fix": "eslint --fix --cache src --ext .ts,.html & stylelint \"**/*.less\" --fix",


You're actually starting eslint as a background task and then stylelint. This will cause eslint to finish after the process has already exited, and cause a zero return code even if eslint found issues.

Suggested change

"lint": "eslint --cache src --ext .ts,.html & stylelint \"**/*.less\"",

"lint:fix": "eslint --fix --cache src --ext .ts,.html & stylelint \"**/*.less\" --fix",

"lint": "eslint --cache src --ext .ts,.html && stylelint \"**/*.less\"",

"lint:fix": "eslint --fix --cache src --ext .ts,.html && stylelint \"**/*.less\" --fix",

Yep, this was a potential problem. Thanks for pointing it out.

Airblader

This PR looks good to me now. Thanks for working on it! I'll have to defer merging to @twalthr.

twalthr

Thanks for the contribution @yangjunhan and the review @Airblader. As before, I don't have much knowledge about the WebUI and need to trust you guys that the changes are valid. It would be good to have a final green build (maybe after a rebase) and all conflicts resolved. Please ping me once this is done and I will merge asap.

Airblader · 2021-09-03T09:44:23Z

@yangjunhan I've also noticed now that you are migrating the package-lock.json back from version 2 to 1. Can you please fix that commit to have it generated as version 2? This requires npm >= 7.

…elint for less

yangjunhan · 2021-09-06T03:16:22Z

@yangjunhan I've also noticed now that you are migrating the package-lock.json back from version 2 to 1. Can you please fix that commit to have it generated as version 2? This requires npm >= 7.

@Airblader Yep, it was caused by my local node/npm versions, and I fixed it. Please have a look to see whether this PR is ready for merging.

yangjunhan · 2021-09-06T07:22:06Z

@twalthr I guess this PR is ready for merging now.

MartijnVisser · 2021-09-06T13:31:00Z

I took the liberty to also do some quick testing in the user interface. The only thing I noticed was when I opened the Logs or the Stdout tabs for both the Job Manager or Task Manager, the Chrome Devtools report multiple 404's. However, the functionality is working as expected.

DevTools failed to load source map: Could not load content for http://localhost:8081/min-maps/vs/loader.js.map: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE
DevTools failed to load source map: Could not load content for http://localhost:8081/min-maps/vs/editor/editor.main.js.map: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE
DevTools failed to load source map: Could not load content for http://localhost:8081/min-maps/vs/editor/editor.main.nls.js.map: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE
DevTools failed to load source map: Could not load content for http://localhost:8081/min-maps/vs/base/worker/workerMain.js.map: Fetch through target failed: Target not supported; Fallback: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE

MartijnVisser

I see that the 404 errors also occurred in previous version (Flink 1.13.2). LGTM

twalthr · 2021-09-06T13:41:01Z

Thanks everyone. Merging...

This closes apache#16902.

rmetzger added the review=description? label Aug 20, 2021

rmetzger added the component=Runtime/WebFrontend label Aug 20, 2021

yangjunhan force-pushed the chore/eslint branch from af560fe to 7df649c Compare August 20, 2021 03:39

Airblader requested changes Aug 20, 2021

View reviewed changes

yangjunhan force-pushed the chore/eslint branch from 7df649c to 7591ced Compare August 20, 2021 08:40

yangjunhan changed the title ~~[FLINK-23113][runtime-web] Migrate from tslint to eslint and add stylint for less~~ [FLINK-23113][runtime-web] Migrate to ESLint, add Stylint and address dependency vulnerabilities Aug 20, 2021

yangjunhan force-pushed the chore/eslint branch 2 times, most recently from bf20750 to b32af96 Compare August 23, 2021 03:09

yangjunhan force-pushed the chore/eslint branch 9 times, most recently from b8be621 to 233e2db Compare August 24, 2021 11:16

Airblader reviewed Aug 25, 2021

View reviewed changes

yangjunhan force-pushed the chore/eslint branch from 233e2db to 60fd053 Compare August 25, 2021 08:47

Airblader requested changes Sep 2, 2021

View reviewed changes

yangjunhan force-pushed the chore/eslint branch from 60fd053 to 0157b9a Compare September 3, 2021 02:26

Airblader approved these changes Sep 3, 2021

View reviewed changes

twalthr approved these changes Sep 3, 2021

View reviewed changes

yangjunhan force-pushed the chore/eslint branch from 0157b9a to bc16f1e Compare September 6, 2021 02:52

子扬 added 4 commits September 6, 2021 10:57

[FLINK-23113][runtime-web] Migrate from tslint to eslint and add styl…

c9c4528

…elint for less

[FLINK-23113][runtime-web] Apply ESLint and Stylelint fixed to files

229ac5a

[FLINK-23114][runtime-web] Fix dependency vulnerabilities

c6aca18

[FLINK-23112][runtime-web] Enforce npm run lint in ci

6d6d751

yangjunhan force-pushed the chore/eslint branch from bc16f1e to 6d6d751 Compare September 6, 2021 03:10

Airblader approved these changes Sep 6, 2021

View reviewed changes

MartijnVisser approved these changes Sep 6, 2021

View reviewed changes

twalthr closed this in f0a0648 Sep 6, 2021

yangjunhan deleted the chore/eslint branch November 1, 2021 09:43

niklassemmler pushed a commit to niklassemmler/flink that referenced this pull request Feb 3, 2022

[FLINK-23112][runtime-web] Enforce npm run lint in ci

f5a2a38

This closes apache#16902.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-23113][runtime-web] Migrate to ESLint, add Stylint and address dependency vulnerabilities #16902

[FLINK-23113][runtime-web] Migrate to ESLint, add Stylint and address dependency vulnerabilities #16902

yangjunhan commented Aug 20, 2021 •

edited

flinkbot commented Aug 20, 2021

flinkbot commented Aug 20, 2021 •

edited

yangjunhan commented Aug 20, 2021

Airblader left a comment

Airblader Aug 20, 2021

Airblader Aug 20, 2021

yangjunhan commented Aug 20, 2021 •

edited

Airblader commented Aug 23, 2021

yangjunhan commented Aug 25, 2021 •

edited

Airblader left a comment

Airblader Aug 25, 2021

yangjunhan Aug 25, 2021

yangjunhan Aug 25, 2021

Airblader Aug 25, 2021

yangjunhan Aug 25, 2021

Airblader Aug 25, 2021

yangjunhan Aug 25, 2021

Airblader left a comment

Airblader Sep 2, 2021

yangjunhan Sep 3, 2021

Airblader left a comment

twalthr left a comment

Airblader commented Sep 3, 2021

yangjunhan commented Sep 6, 2021 •

edited

yangjunhan commented Sep 6, 2021

MartijnVisser commented Sep 6, 2021

MartijnVisser left a comment

twalthr commented Sep 6, 2021

		@@ -0,0 +1,2 @@
		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		export type SafeAny = any;

		"lint": "eslint --cache src --ext .ts,.html & stylelint \"*/.less\"",
		"lint:fix": "eslint --fix --cache src --ext .ts,.html & stylelint \"*/.less\" --fix",

[FLINK-23113][runtime-web] Migrate to ESLint, add Stylint and address dependency vulnerabilities #16902

[FLINK-23113][runtime-web] Migrate to ESLint, add Stylint and address dependency vulnerabilities #16902

Conversation

yangjunhan commented Aug 20, 2021 • edited

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Aug 20, 2021

Automated Checks

Review Progress

flinkbot commented Aug 20, 2021 • edited

CI report:

yangjunhan commented Aug 20, 2021

Airblader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangjunhan commented Aug 20, 2021 • edited

Airblader commented Aug 23, 2021

yangjunhan commented Aug 25, 2021 • edited

Airblader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Airblader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Airblader left a comment

Choose a reason for hiding this comment

twalthr left a comment

Choose a reason for hiding this comment

Airblader commented Sep 3, 2021

yangjunhan commented Sep 6, 2021 • edited

yangjunhan commented Sep 6, 2021

MartijnVisser commented Sep 6, 2021

MartijnVisser left a comment

Choose a reason for hiding this comment

twalthr commented Sep 6, 2021

yangjunhan commented Aug 20, 2021 •

edited

flinkbot commented Aug 20, 2021 •

edited

yangjunhan commented Aug 20, 2021 •

edited

yangjunhan commented Aug 25, 2021 •

edited

yangjunhan commented Sep 6, 2021 •

edited