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

Fix Component Governance alerts #41485

Merged
merged 5 commits into from May 3, 2022
Merged

Fix Component Governance alerts #41485

merged 5 commits into from May 3, 2022

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented May 2, 2022

Will also need dotnet/spa-templates#47

ws@^7.2.3, ws@^7.4.5, ws@^7.4.6, ws@~8.2.3:
version "7.5.7"
resolved "https://registry.yarnpkg.com/ws/-/ws-7.5.7.tgz#9e0ac77ee50af70d58326ecff7e85eb3fa375e67"
integrity sha512-KMvVuFzpKBuiIXW3E4u3mySRO2/mCHSyZDJQM5NQ9Q9KHWHWh0NHgfbRMLLrceUK5qAL4ytALJbpRMjixFZh8A==
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading this correctly, it looks scary. Does this mean that a package requesting ws v8.2.3 will instead get v7.5.7❔ If yes, is there a way to map v7.x.y to v7.5.7 but still have v8.2.3 map to itself❔

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it does mean that, and I'm not sure if there's a way to achieve that. I'm trying ^ here because I've run into problems with resolving versions using >= as that often updates things too much and leads to build breaks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use >= instead of ^, after looking at other branches I'm thinking that >= breaks things less often than I'd thought.

Comment on lines +3553 to +3554
ws@>=7.4.6, ws@^7.2.3, ws@^7.4.5, ws@~8.2.3:
version "8.6.0"
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is better but now the 7.x.y dependencies are getting updated across a major version boundary. @BrennanConroy is JS testing enough to catch any problems that may cause❔

Copy link
Member

Choose a reason for hiding this comment

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

This version is only used for a single test AFAIK and it's sort of a silly test IMO.

The majority of our tests that use WS actually get WS from https://github.com/dotnet/aspnetcore/tree/main/src/SignalR/clients/ts/signalr which is using 7.4.6

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

WFM as long as the big ws version bumps definitely don't break anything

@@ -49,6 +49,7 @@
},
"devDependencies": {},
"resolutions": {
"url-parse": ">=1.5.6"
"url-parse": ">=1.5.6",
"ws": ">=7.4.6"
Copy link
Member

Choose a reason for hiding this comment

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

nit: This might as well be >=8.6.0 based on the yarn.lock file

@mkArtakMSFT mkArtakMSFT added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 3, 2022
@wtgodbe
Copy link
Member Author

wtgodbe commented May 3, 2022

Spa-templates are now updated in this PR (CC @mkArtakMSFT @MackinnonBuck)

@wtgodbe
Copy link
Member Author

wtgodbe commented May 3, 2022

@javiercn @MackinnonBuck @HaoK we've got a template test failure after updating dependencies in spa-templates, can you take a look?

Templates.Test.BaselineTest.Template_Produces_The_Right_Set_Of_FilesAsync(arguments: "new react", expectedFiles: ["ClientApp/public/favicon.ico", "ClientApp/public/index.html", "ClientApp/public/manifest.json", "ClientApp/src/components/Counter.js", "ClientApp/src/components/FetchData.js", ...])

Assert.Contains() Failure
Not found: ClientApp/src/AppRoutes.js
In value: String[] ["ClientApp/public/favicon.ico", "ClientApp/public/index.html", "ClientApp/public/manifest.json", "ClientApp/src/components/Counter.js", "ClientApp/src/components/FetchData.js", ...]

@MackinnonBuck
Copy link
Member

@wtgodbe AppRoutes.js was added to the React template as part of the dependency updates, so the baselines just need to be regenerated.

@wtgodbe wtgodbe enabled auto-merge (squash) May 3, 2022 20:15
@wtgodbe wtgodbe merged commit bc42cad into dotnet:main May 3, 2022
@ghost ghost added this to the 7.0-preview5 milestone May 3, 2022
@wtgodbe wtgodbe deleted the wtgodbe/CG422 branch May 3, 2022 22:14
@RichardSunMS
Copy link

@wtgodbe, @dougbu, I am having another Governance issue: #41729, can you guys help take a look?

@ghost
Copy link

ghost commented May 18, 2022

Hi @RichardSunMS. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants