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

Npm update packages #233

Merged

Conversation

BluThaitanium
Copy link
Contributor

Updates the npm packages through the make check_docs_links command.

Phase 1:
Finds outdated npm packages
If outdated npm packages are found, asks to audit

Phase 2:
If auditing, runs npm audit to attempt to update non-breaking changes
Checks if there are outdated packages again

Phase 3:
If there are still outdated packages, manual inspection is needed
Gives steps on how to manually determine which packages are breaking

@BluThaitanium BluThaitanium marked this pull request as draft October 11, 2021 20:23
@BluThaitanium BluThaitanium marked this pull request as ready for review October 11, 2021 20:23
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Thanks Phu! I have a few comments, questions below

Makefile Outdated Show resolved Hide resolved
tools/python/verify_npm_packages.py Outdated Show resolved Hide resolved
tools/python/verify_npm_packages.py Show resolved Hide resolved
@BluThaitanium BluThaitanium force-pushed the npm_update_packages branch 4 times, most recently from 608e30d to a51c6e5 Compare October 27, 2021 17:26
@BluThaitanium BluThaitanium force-pushed the npm_update_packages branch 3 times, most recently from d24d5f1 to 41b830f Compare October 27, 2021 20:20
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Thank you Phu I found a few small problem that still need to be addressed

Makefile Outdated Show resolved Hide resolved
tools/python/verify_npm_packages.py Outdated Show resolved Hide resolved


def verify_npm_packages():
check_outdated = run("npm outdated", cwd="./dashboard/origin-mlx/", shell=True)
Copy link
Member

@ckadner ckadner Oct 29, 2021

Choose a reason for hiding this comment

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

I wonder if we should limit this function to only report vulnerabilities, as opposed to checking for outdated versions?

If we want to run this during our CI, I want to make sure that a new PR only gets a failed check ❌ on a PR, if there is an actual vulnerability that needs to be addressed.

tools/python/verify_npm_packages.py Outdated Show resolved Hide resolved
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Thanks @BluThaitanium -- just need to add the return code, so that Make can fail if there are outdated packages

tools/python/verify_npm_packages.py Outdated Show resolved Hide resolved
@ckadner ckadner added the RCOS Potential work items for RCOS student interns label Dec 1, 2021
@ckadner ckadner assigned ckadner and unassigned BluThaitanium Dec 2, 2021
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Assigning to me for now since I still need to test it

@ckadner
Copy link
Member

ckadner commented Dec 9, 2021

I finally took the time to give your PR another try. I struggled for an hour with Node/npm. Seems like the latest brew-installed node/npm version did not work, but a direct install from https://nodejs.org/en/#home-downloadhead worked.

@ckadner
Copy link
Member

ckadner commented Dec 9, 2021

I do however get this error:

[mlx_ckadner] (pr-233 *)$ make check_npm_packages

Package                    Current    Wanted   Latest  Location                               Depended by
@material-ui/core            3.9.4     3.9.4   4.12.3  node_modules/@material-ui/core         origin-mlx
@material-ui/icons           3.0.2     3.0.2   4.11.2  node_modules/@material-ui/icons        origin-mlx
@types/jest                24.0.11   24.0.11   27.0.2  node_modules/@types/jest               origin-mlx
@types/js-cookie             2.2.7     2.2.7    3.0.0  node_modules/@types/js-cookie          origin-mlx
@types/js-yaml              3.12.7    3.12.7    4.0.4  node_modules/@types/js-yaml            origin-mlx
@types/node               14.17.17  14.17.32  16.11.6  node_modules/@types/node               origin-mlx
@types/react                16.8.7    16.8.7  17.0.33  node_modules/@types/react              origin-mlx
@types/react-dom            16.8.2    16.8.2  17.0.10  node_modules/@types/react-dom          origin-mlx
@types/react-router-dom      4.3.5     4.3.5    5.3.2  node_modules/@types/react-router-dom   origin-mlx
@types/react-sidebar         3.0.1     3.0.2    3.0.2  node_modules/@types/react-sidebar      origin-mlx
@types/styled-components     4.4.3     4.4.3   5.1.15  node_modules/@types/styled-components  origin-mlx
codemirror                  5.62.3    5.63.3   5.63.3  node_modules/codemirror                origin-mlx
js-cookie                    2.2.1     2.2.1    3.0.1  node_modules/js-cookie                 origin-mlx
js-yaml                     3.14.1    3.14.1    4.1.0  node_modules/js-yaml                   origin-mlx
react                      16.14.0   16.14.0   17.0.2  node_modules/react                     origin-mlx
react-codemirror2            5.1.0     5.1.0    7.2.1  node_modules/react-codemirror2         origin-mlx
react-dom                  16.14.0   16.14.0   17.0.2  node_modules/react-dom                 origin-mlx
react-markdown               4.3.1     4.3.1    7.1.0  node_modules/react-markdown            origin-mlx
react-router-dom             4.3.1     4.3.1    5.3.0  node_modules/react-router-dom          origin-mlx
react-scripts                3.4.4     3.4.4    4.0.3  node_modules/react-scripts             origin-mlx
reactjs-popup                1.5.0     1.5.0    2.0.5  node_modules/reactjs-popup             origin-mlx
reactstrap                  8.10.0    8.10.1    9.0.0  node_modules/reactstrap                origin-mlx
remark-gfm                   2.0.0     2.0.0    3.0.0  node_modules/remark-gfm                origin-mlx
styled-components            4.4.1     4.4.1    5.3.3  node_modules/styled-components         origin-mlx
typescript                  3.9.10    3.9.10    4.4.4  node_modules/typescript                origin-mlx

npm WARN audit request to https://registry.npmjs.org/-/npm/v1/security/audits/quick failed, reason: connect ETIMEDOUT 2606:4700::6810:1723:443
npm ERR! audit endpoint returned an error
Traceback (most recent call last):
  File "tools/python/verify_npm_packages.py", line 110, in <module>
    verify_npm_packages()
  File "tools/python/verify_npm_packages.py", line 89, in verify_npm_packages
    \rRun {colorText.BLUE}make update_npm_packages{colorText.END} to secure/update\n'''
IndexError: list index out of range
make: *** [check_npm_packages] Error 1

@ckadner
Copy link
Member

ckadner commented Dec 9, 2021

npm WARN audit request to https://registry.npmjs.org/-/npm/v1/security/audits/quick failed, reason: connect ETIMEDOUT 2606:4700::6810:1723:443
npm ERR! audit endpoint returned an error

On my other laptop I did not get the error. So I am gonna write this off as a fluke :-)

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

I think your changes are good.

Could you also update the package-lock.json file with your PR?

@mlx-bot
Copy link
Collaborator

mlx-bot commented Dec 9, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BluThaitanium
To complete the pull request process, please ask for approval from ckadner after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ckadner ckadner merged commit 74a13ab into machine-learning-exchange:main Dec 9, 2021
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD RCOS Potential work items for RCOS student interns UI User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants