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

Upgraded yargs package to fix #7444 #7476

Closed
wants to merge 1 commit into from
Closed

Upgraded yargs package to fix #7444 #7476

wants to merge 1 commit into from

Conversation

c16a
Copy link
Contributor

@c16a c16a commented Apr 1, 2020

What it does

Upgades the version of yargs package to fix https://snyk.io/vuln/SNYK-JS-YARGSPARSER-560381

How to test

Tests can be run with yarn test from the examples/browser directory.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the dependencies pull requests that update a dependency file label Apr 1, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified that the new dependency version for yargs is license compatible using the steps as described here.

@c16a
Copy link
Contributor Author

c16a commented Apr 1, 2020

@vince-fugnitto let me know if this is good to go.

@vince-fugnitto
Copy link
Member

@vince-fugnitto let me know if this is good to go.

A couple of things before proceeding:

Screen Shot 2020-04-01 at 9 25 32 AM
  • Please be sure to squash all commits. In general, we ask that a single commit be present in a pull-request unless commits can be logically separated.

@c16a
Copy link
Contributor Author

c16a commented Apr 1, 2020

@vince-fugnitto - done with both. Can you check if my ECA shows up now?

@vince-fugnitto
Copy link
Member

@vince-fugnitto - done with both. Can you check if my ECA shows up now?

Yes, the ECA looks good now thank you!
I'll wait for Travis to be green before merging, thank you for your first contribution 👍

Screen Shot 2020-04-01 at 9 54 27 AM

@c16a
Copy link
Contributor Author

c16a commented Apr 1, 2020

@vince-fugnitto - thanks! Looks everything is good now.

vince-fugnitto
vince-fugnitto previously approved these changes Apr 1, 2020
@vince-fugnitto
Copy link
Member

vince-fugnitto commented Apr 1, 2020

@vince-fugnitto - thanks! Looks everything is good now.

Was there a reason for not updating other extensions?
We also have yargs in these places:

"yargs": "^11.1.0"

"yargs": "^11.1.0"

cli will be in fact a devDependency in the end, but not electron as it is mandatory for electron-based theia applications. In order to align, I think we should update both of them similarly to core.

@vince-fugnitto vince-fugnitto self-requested a review April 1, 2020 16:54
@vince-fugnitto vince-fugnitto dismissed their stale review April 1, 2020 16:54

Additional changes required.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@paul-marechal
Copy link
Member

paul-marechal commented Apr 1, 2020

I noticed too late that it wasn't updated in the other places, but given what yarn resolved with your first modification, you should just update the other places and it will be good to go!

@akosyakov
Copy link
Member

@marechal-p Anything else should be done to merge it?

@vince-fugnitto
Copy link
Member

@marechal-p Anything else should be done to merge it?

We should update the other occurrences of yargs in other extensions, we can do so by updating the pull-request ourselves as there hasn't been much activity lately.

@akosyakov
Copy link
Member

@vince-fugnitto fine with me

Signed-off-by: Chaitanya Munukutla <chaitanya.m61292@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants