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

Ensure plugins work with Dashboards Node v14.18.2 #875

Closed
Tracked by #118
boktorbb opened this issue Dec 17, 2021 · 8 comments · Fixed by #928
Closed
Tracked by #118

Ensure plugins work with Dashboards Node v14.18.2 #875

boktorbb opened this issue Dec 17, 2021 · 8 comments · Fixed by #928
Assignees
Labels

Comments

@boktorbb
Copy link

Dashboards has upgraded Node to v14.18.2 and this is for plugin teams to verify that their plugins work with the new version upgrade. The Dashboards branch running the upgraded node is feature/node14.

A list of things to check is:

  • Your plugin's node version is bumped to 14.18.2
  • Your plugin's @types/node package is bumped to ^14.17.32 if it's used
  • Plugin builds and runs on Dashboards
  • All of your plugin's tests pass
  • Your GitHub workflows are not broken by the node version change and updated if they do
  • Any sanity/smoke testing is successful
@hsiang9431-amzn
Copy link
Contributor

@types/node is not used in our code and the CI using latest Dashboards core is passing. Not seeing any action shall be taken for this one

@davidlago
Copy link

@hsiang9431-amzn CI is not running with node 14.18.2 as far as I know. Our CI in main runs against 1.3.0 of Dashboards, and that still uses node10

@davidlago davidlago reopened this Jan 8, 2022
@hsiang9431-amzn
Copy link
Contributor

yarn lint

Seeing error like this on yarn lint when changing the version to 2.0.0 for testing against Dashboards on main branch

$ yarn run lint:es && yarn run lint:sass
$ node ../../scripts/eslint
Error: .eslintrc.js#overrides[0]:
	Configuration for rule "@osd/eslint/require-license-header" is invalid:
	Value {"license":"\n/*\n *   Copyright OpenSearch Contributors\n *\n *   Licensed under the Apache License, Version 2.0 (the \"License\").\n *   You may not use this file except in compliance with the License.\n *   A copy of the License is located at\n *\n *       http://www.apache.org/licenses/LICENSE-2.0\n *\n *   or in the \"license\" file accompanying this file. This file is distributed\n *   on an \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either\n *   express or implied. See the License for the specific language governing\n *   permissions and limitations under the License.\n */\n"} should NOT have additional properties.

Unit test yarn test:jest_ui

Success without error

Integration test test:jest_server

No corresponding backend plugin can be tested against. need further work with backend owner

@davidlago
Copy link

I'm not sure how you're testing this, but I've tested with a 2.0.0 of dashboards and I get this:

yarn run lint:es && yarn run lint:sass
yarn run v1.22.17
$ node ../../scripts/eslint
✨  Done in 13.60s.
yarn run v1.22.17
$ node ../../scripts/sasslint
✨  Done in 1.31s.

after changing line 31 in .eslintrc.js from license: LICENSE_HEADER, to licenses: [LICENSE_HEADER],

@davidlago
Copy link

I've created #892 to start getting ready our 2.0 version, pending fixing some issues in the security plugin (backend) so that we can use its 2.0.0 artifacts in our CI.

@davidlago
Copy link

From @hsiang9431-amzn

It turns out to be the new version of node does not recognize the old implementation of a regular expression. This line of change is what needs to be done for fixing it
https://github.com/hsiang9431-amzn/security-dashboards-plugin/pull/16/files#diff-d8221b62ee0c661ced285c3443a955d1a7d4a5b663e0125bdaee45f866d3c76fL47

-  return !/[\p{C}%]/u.test(resourceName) && resourceName.length > 0;
+  const exp = new RegExp("[\p{C}%]", 'u');
+  return !exp.test(resourceName) && resourceName.length > 0;

@DarshitChanpura DarshitChanpura self-assigned this Mar 21, 2022
@DarshitChanpura
Copy link
Member

From @hsiang9431-amzn

It turns out to be the new version of node does not recognize the old implementation of a regular expression. This line of change is what needs to be done for fixing it https://github.com/hsiang9431-amzn/security-dashboards-plugin/pull/16/files#diff-d8221b62ee0c661ced285c3443a955d1a7d4a5b663e0125bdaee45f866d3c76fL47

-  return !/[\p{C}%]/u.test(resourceName) && resourceName.length > 0;
+  const exp = new RegExp("[\p{C}%]", 'u');
+  return !exp.test(resourceName) && resourceName.length > 0;

so new RegExp("[\p{C}%]", 'u'); still fails the linter check, and thus I have replaced it with new RegExp(/[\p{C}%]/u);

@DarshitChanpura
Copy link
Member

this issue is covered in this PR: #928

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants