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

Ability to start OS Dashboards with a newer and compatible nodejs version #2091

Closed
wants to merge 177 commits into from

Conversation

sipopo
Copy link

@sipopo sipopo commented Aug 8, 2022

Description

Resolve bug the application doesn't start with nodejs version 14.20.0

Issues Resolved

[BUG] Doesn't start with nodejs 14.20.0 #2088

@sipopo sipopo requested a review from a team as a code owner August 8, 2022 16:32
kavilla
kavilla previously requested changes Aug 8, 2022
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Added details to #2088, this logic is intentional.

@sipopo
Copy link
Author

sipopo commented Aug 9, 2022

I added warning message for minor check

@sipopo
Copy link
Author

sipopo commented Aug 12, 2022

Hi, It is my first pull request in github. Do I need something to do else?

Sorry for my question...

@smortex
Copy link

smortex commented Aug 12, 2022

#2101 was merged and update nodejs to 14.20.0, but the point of this PR/issue #2088 remains. I would suggest clarifying this by updating this Issue and PR subject to something like "Allow using a newer minor version of nodejs than the bundled one" or "Fix startup when using a newer minor version of nodejs then the bundled one" (since the issue is not 14.20.0 specific anymore).

At the bottom of the page, you will see 2 failing tests:

screenshot

Click "Details" to see what was wrong, fix it and update the PR accordingly (see bellow for some hints). Each project is different, but the idea is usually to external contributors to work on their code and have automatic feedback, and reviewers will wait for all tests to be green before considering merging code. Until then, they may still of course help you.

You mention this is your first PR (awesome!), so it's good to know that any update you do in your branch will update this PR: you can re-write your commits and force push, and the PR will automagically update!

For example assuming you want to move your commits on top of master, sign them all to make the "Developer Certificate of Origin" check green, and rework them in a single commit (squash them), you can, from your working branch:

git fetch origin       # Download the latest code we have here
git rebase origin/main --signoff --interactive # Interactively move your commits on top of the main branch ensuring they are all signed

This will bring your editor listing all commits in your branch. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will pop again and allow you to combine all the commit messages in a single one.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

@ashwin-pc
Copy link
Member

@sipopo will you be able to add a signoff to your change? We cannot accept your change unless we have that. The PR otherwise looks great! @smortex's comment has detailed instructions on how to do so

fbaligand and others added 11 commits September 14, 2022 18:44
…unction (opensearch-project#2192)

In pipeline building, this PR adds `visConfig.title` and `uiState` properties for "visualization" function.

This aims 2 goals:
-    To be consistent with pipelines built with "buildPipelineVisFunction".
-    To provide "title" and "uiState" informations for visualizations.

This is currently a missing information for community plugins.

Signed-off-by: fbaligand <[email protected]>

Signed-off-by: fbaligand <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
…ch-project#2277)

Original implementation incorrectly assumed the return list of
nodes was an object array.
This PR:
opensearch-project#2232

Addressed the return but didn't catch the nodes.find in the return
which is a function for an array.

Also, refactors to return a list of node_ids because the original
implementation indicated that it should but it can return node ids
but it never did. It only returned `null` or `_local`, the problem
with this approach is that it doesn't expect valid node version
with different DIs or filter out nodes when we pass `null` as the node
ID for the node info call because it was fan out the request to all
nodes.

Now this function will return `_local` if all the nodes share the
same cluster_id using a greedy approach since we can assume it is
all the same version.

Will return node ids, if the cluster_id are different so it will
pass a CSV to the node info call and return the info for those nodes.
And null if no cluster_id is present, ie, fan out the response.

Original issue:
opensearch-project#2203

Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
* Update API Specs for dev console
* Remove deprecated field - master_timeout

Signed-off-by: Kristen Tian <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
* Working histogram

Signed-off-by: Ashwin Pc <[email protected]>

* Adds histogram options

Signed-off-by: Ashwin Pc <[email protected]>

* Adds line chart

Signed-off-by: Ashwin Pc <[email protected]>

* Adds area chart

Signed-off-by: Ashwin Pc <[email protected]>

* updates nav sizes

Signed-off-by: Ashwin Pc <[email protected]>

* adds resizeable container to right nav

Signed-off-by: Ashwin Pc <[email protected]>

* Misc fixes

Signed-off-by: Ashwin Pc <[email protected]>

* Adds chart switcher test

Signed-off-by: Ashwin Pc <[email protected]>

* Github comment feedback

Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: Ashwin Pc <[email protected]>

* Updates copy

Signed-off-by: Ashwin Pc <[email protected]>

Signed-off-by: Ashwin Pc <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
* fixes time series for new chart types

Signed-off-by: Ashwin Pc <[email protected]>

* moves translate to source string

Signed-off-by: Ashwin Pc <[email protected]>

Signed-off-by: Ashwin Pc <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Ran npx browserslist@latest --update-db to update caniuse package
so the integration tests will pass

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Signed-off-by: Louis Chu <[email protected]>

* Add v2.3.0 release notes

Signed-off-by: Sergey V. Osipov <[email protected]>
)

Add readme for saved object and index pattern relationship

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
For issue [opensearch-project#2223](opensearch-project#2223), in order to solve the slow compilation time that is caused by changing from node-sass to dart-sass, one possible solution is to add a node-fibers package. Pulling from the sass official website, "to avoid this performance hit, render() can use the fibers package to call asynchronous importers from the synchronous code path". And i have validated the performance improvement: the first compilation changed to 76s from the previous 409s.

However, node-fiber package has reached end of life with Node 16. If the performance issue is a problem, we can consider stay on Node 14 and use node-fiber for now, and remove fibers when migrating to Node 16 later.

Issue resolved:
opensearch-project#2223

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
To save resources and job runners, some PRs do not need to run the full test suite. For PRs and pushes with markdown changes only, the build and test workflow will be skipped. (Linter test should be okay to skipped since it will automatically run with every commit). For PRs and pushes with markdown changes along with other changes, the build and test workflow will not be skipped.

Issue Resolved:
opensearch-project#1214

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
@sipopo
Copy link
Author

sipopo commented Sep 14, 2022

Sorry for late. I signed my commit.

@sipopo
Copy link
Author

sipopo commented Sep 15, 2022

Sorry for my misunderstanding. I returned my changes to project )

@ashwin-pc
Copy link
Member

@sipopo Thanks for making the change!

Signed-off-by: Sergey V. Osipov <[email protected]>
@ashwin-pc
Copy link
Member

@sipopo Looks like your change is breaking a version check test.

@joshuarrrr joshuarrrr requested a review from kavilla October 4, 2022 20:35
AMoo-Miki and others added 21 commits December 16, 2022 11:54
Bumps [loader-utils](https://github.com/webpack/loader-utils) from 2.0.3 to 2.0.4.
- [Release notes](https://github.com/webpack/loader-utils/releases)
- [Changelog](https://github.com/webpack/loader-utils/blob/v2.0.4/CHANGELOG.md)
- [Commits](webpack/loader-utils@v2.0.3...v2.0.4)

---
updated-dependencies:
- dependency-name: loader-utils
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Sergey Osipov <[email protected]>
Fixes opensearch-project#2996, opensearch-project#2353

- Bump `leaflet-vega` to `^0.9.0` - they merged in our upstream PR and made a release
- Fix the parameter name for passing `vega.parse` options.

Signed-off-by: Miki <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
Basic data points
[OSD] 16 submitted PRs (https://github.com/opensearch-project/OpenSearch-Dashboards/pulls/manasvinibs)
[OSD] 74 reviewed PRs (https://github.com/opensearch-project/OpenSearch-Dashboards/issues?q=reviewed-by%3Amanasvinibs)
[OSD] 28 issues involved (https://github.com/opensearch-project/OpenSearch-Dashboards/issues?page=1&q=involves%3Amanasvinibs+is%3Aissue)

Highlight
Mana is assisting with extensions project which will be the next evolution of extending core functionality from OpenSearch Dashboards
Mana implemented opensearch-project#2734 which allows for a huge quality of life for local development for external plugin developers to utilize snapshots with a single CLI command compared to before when they would had to pull down OpenSearch build, install their plugin on OpenSearch, and ensure the proper configurations. This has caused historically problems when plugin teams do development and miss some steps per their onboard documentation/PR suggestion and get different results than expected.
Mana has assisted reviewing PRs providing great insight on BWC tests, BWC in general, and the release process.
Mana has added documentation from insight she has gained within the informal dev doc repo https://cptnb.github.io/opensearch-dashboards-dev-docs/ ensuring the spread of knowledge.

Signed-off-by: Kawika Avilla <[email protected]>

Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
…-project#2896)

* Add global data persistence for vis builder

Persist filters, time range, time refresh interval for vis builder when we
refresh or navigate to other apps such as dashboard, discover, timeline and visualize

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
…true (opensearch-project#3020)

`no-restricted-paths` compares source files and import statements, and their membership in restricted zones. However, when `allowSameFolder` is true, it failed to remove a trailing slash before validation which results in a false-positive.

Signed-off-by: Miki <[email protected]>

Signed-off-by: Miki <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
* test connection intial code

Signed-off-by: mpabba3003 <[email protected]>

* error handling

Signed-off-by: mpabba3003 <[email protected]>

* refactor

Signed-off-by: mpabba3003 <[email protected]>

* removing get cluster info dependency

Signed-off-by: mpabba3003 <[email protected]>

* refactor test connection

Signed-off-by: mpabba3003 <[email protected]>

* adding test cases and test connection on edit datasource

Signed-off-by: mpabba3003 <[email protected]>

* adding changelog comment

Signed-off-by: mpabba3003 <[email protected]>

* fixing bug on edit datasource page

Signed-off-by: mpabba3003 <[email protected]>

* refactor based on PR comments

Signed-off-by: mpabba3003 <[email protected]>

Signed-off-by: mpabba3003 <[email protected]>
* [Chore] Add 2.4.1 release notes

Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
…t#2918)

Currently, the new table can not format Url. If we
set to use URL format in index pattern field, table
will display it as string.

In this PR, we switch the format from string to html.
To make html understandable by react as a DOM element,
we use dangerouslySetInnerHTML to transform it. For the
security, since the content is not from random input but
fetched from stored data, we should be safe as long as
data is not messed.

To provide more security protection, we also add dompurify
package to sanitize the html content.

Issue Resolved:
opensearch-project#2905

Signed-off-by: Anan Zhuang <[email protected]>

Signed-off-by: Anan Zhuang <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
* Removes manual resolution of `axios`.

Signed-off-by: Tommy Markley <[email protected]>

Signed-off-by: Tommy Markley <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
* The minimatch resolution was no longer necessary after the upstream
library that depended on v3.0.4 was removed in opensearch-project#2711.

Signed-off-by: Tommy Markley <[email protected]>

Signed-off-by: Tommy Markley <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
Signed-off-by: Kristen Tian <[email protected]>

Signed-off-by: Kristen Tian <[email protected]>
Co-authored-by: Miki <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
Signed-off-by: dblock <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
Bumps [decode-uri-component](https://github.com/SamVerschueren/decode-uri-component) from 0.2.0 to 0.2.2.
- [Release notes](https://github.com/SamVerschueren/decode-uri-component/releases)
- [Commits](SamVerschueren/decode-uri-component@v0.2.0...v0.2.2)

---
updated-dependencies:
- dependency-name: decode-uri-component
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
Signed-off-by: Sergey Osipov <[email protected]>
@ashwin-pc
Copy link
Member

So i pulled down the changes to validate if this works as expected only to run into a very interesting behavior. @sipopo @AMoo-Miki How did the two of you validate this functionality? Because when i run the yarn commands, it turns out that the first line of defense is node and package.json itself.

We have an engines key in the package.json file that fails first when we try to use any other minor or patch version of node. It does not even get to node check. The only two ways to get past this is to update the engine requirements to be something like ^14.20.0 or pass the --ignore-engines flag. Did either of you run into this while testing the feature? Also if you use the former option of specifying the node version using the caret symbol, the regex that you use is no longer valid. Here's one way to fix that:

  1. Delete line 37 where we prefix v and rename rawRequiredVersion to requiredVersion. We know that this will not be null for our usecases so this is a safe assumption. And to be honest, i'm not sure that line makes any sense.
  2. Modify the version regex to /^[v~=\^]+(\d+)\.(\d+)\.(\d+)/ to handle v, ^, ~ and = prefixes

With these changes, the rest of the code works as expected. I think this is also why @kavilla and @miki might have called out the initial change did not work.

@@ -1,3 +1,4 @@
=======
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this change?

@abbyhu2000 abbyhu2000 added v2.6.0 and removed v2.5.0 'Issues and PRs related to version v2.5.0' labels Jan 6, 2023
@AMoo-Miki AMoo-Miki self-requested a review February 9, 2023 00:51
@AMoo-Miki
Copy link
Collaborator

So i pulled down the changes to validate if this works as expected only to run into a very interesting behavior. @sipopo @AMoo-Miki How did the two of you validate this functionality? Because when i run the yarn commands, it turns out that the first line of defense is node and package.json itself.

The original idea was that this piece would relax the requirement and then engines.node would use 14.20. I felt that didn't eliminate the restrictions enough and hence I implemented this very differently in #3402.

@sipopo I would have loved to add you as a co-author but due to the missing DCO in this PR, I couldn't. Thanks for all the effort you put in here; know that you were a big part of the motivation to get this done.

@AMoo-Miki AMoo-Miki closed this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.