-
Notifications
You must be signed in to change notification settings - Fork 424
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
TEZ-4422 : [CVE-2021-43138] Upgrade async from 2.3.0 to 2.6.4 #217
Conversation
…the vulnerability. Also an upgrade of yarn version to 1.6.0 and frontend maven plugin to 1.8.0 was done.
@abstractdog Can you please review this PR. As suggested by you, I am raising individual PR's for the TEZ-4419 parent issue. |
💔 -1 overall
This message was automatically generated. |
@@ -67,7 +67,7 @@ | |||
<roaringbitmap.version>0.7.45</roaringbitmap.version> | |||
<protoc.path>${env.PROTOC_PATH}</protoc.path> | |||
<scm.url>scm:git:https://gitbox.apache.org/repos/asf/tez.git</scm.url> | |||
<frontend-maven-plugin.version>1.4</frontend-maven-plugin.version> | |||
<frontend-maven-plugin.version>1.8.0</frontend-maven-plugin.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this related to async upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please confirm this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abstractdog I confirm this change
@@ -374,7 +374,7 @@ | |||
</goals> | |||
<configuration> | |||
<nodeVersion>${nodeVersion}</nodeVersion> | |||
<yarnVersion>v0.21.3</yarnVersion> | |||
<yarnVersion>v1.6.0</yarnVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this related to async upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please confirm this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abstractdog I confirm this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, this looks like a major version change of yarn, can you please let me know how is this related to the CVE patch (or is it necessary)?
I prefer doing version changes in separate patches if they're not closely related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abstractdog The yarnVersion was changed since the selective dependency feature is only supported in yarn 1.0 and above. And yarnVersion of 1.6.0 is compatible with frontend maven plugin 1.8.0. That's why I did this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, thanks for clarifying @amanraj2520 !
lodash@^4.14.0: | ||
version "4.17.4" | ||
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.4.tgz#78203a4d1c328ae1d86dca6460e369b57f4055ae" | ||
lodash@^4.17.14: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this related to async upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abstractdog Yes these are related to async upgrade. The yark.lock file is automatically updated based on package.json.
left minor comments |
@abstractdog @guptanikhil007 Should we first merge the Ember3 upgrade or push these vulnerability fixes first? |
Yes I think we are good with these changes from our side. I have run tests locally as well with this PR. According to me we can go ahead with this merge @abstractdog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
I'm merging in 12 hours if there are no further comments
@abstractdog Can we please merge this PR. I will raise PR's for remaining vulnerabilities as well after this. |
@abstractdog Just a gentle reminder, can you please merge this PR |
merged, sorry for the delay! |
Upgrade async from 2.3.0 to 2.6.4 to fix the vulnerability. Also an upgrade of yarn version to 1.6.0 and frontend maven plugin to 1.8.0 was done along with this change. Link to JIRA : https://issues.apache.org/jira/browse/TEZ-4422
Link to parent JIRA : https://issues.apache.org/jira/browse/TEZ-4419
RFC documentation : https://github.com/yarnpkg/rfcs/blob/master/implemented/0000-selective-versions-resolutions.md