Skip to content

Conversation

@akshita31
Copy link
Contributor

The UpdateDependencies script used to match the last part of the url to match the urls against the runtimeDependencies. However the urls for omnisharp and razor include the version as well, hence the names would never directly match, so added some logic that trims the versions before performing the matching.

Also update the version in the installPath, installTestPath and the defaults property which is present in the package.json

@codecov
Copy link

codecov bot commented Jan 5, 2019

Codecov Report

Merging #2779 into master will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2779      +/-   ##
==========================================
+ Coverage   65.04%   65.13%   +0.08%     
==========================================
  Files         104      104              
  Lines        4506     4506              
  Branches      654      654              
==========================================
+ Hits         2931     2935       +4     
+ Misses       1389     1386       -3     
+ Partials      186      185       -1
Flag Coverage Δ
#integration 52.72% <ø> (+0.08%) ⬆️
#unit 85.56% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/features/diagnosticsProvider.ts 79.22% <0%> (+1.29%) ⬆️
src/omnisharp/delayTracker.ts 73.68% <0%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0213350...45df3c1. Read the comment docs.

@akshita31
Copy link
Contributor Author

akshita31 commented Jan 5, 2019

@NTaylorMullen I tried to run this script using
a razor url from one of the previous PR's. The script is currently dependent on regex matching and can run only if we have specified a "release" version in the NEW_DEPS_VERSION argument in launch.json. However my observation is that in the razor case, we dont have a fallback url or any specific thing that needs to update the version hence just specifiying 1.0.0 did work and updated the url and the integrity for the packages.

@NTaylorMullen
Copy link

Could you share some of the context around this script? Not quite sure I understand all this goodness

@akshita31
Copy link
Contributor Author

Whenever we have to update the dependencies in package.json we used to manually modify the urls in package.json so far. With a recent PR from @gregg-miskelly - #2766, we can use a script to do that.

However as I have stated in the description of this PR, there are some subtle differences in the url that is used by the debugger packages and the ones used by omnisharp and razor due to which it would not work for us. In this PR I have made changes so that we can use this script as well. Does that make sense ?

@akshita31
Copy link
Contributor Author

This is basically a gulp task where we can add the urls in the launch.json and it would do all the matching of the names and replace the urls and the other things like installPath using the specified url and version.

@NTaylorMullen
Copy link

Ah, I see. Ya we don't currently have a fallback url available for the last release (blame Azure Pipelines retention policy 😢). Am I reading this correctly that this also calculates the checksum for dependencies? If so that's doppppeee

@gregg-miskelly
Copy link
Contributor

@NTaylorMullen Correct. The script will -

  • Make sure all the new URLs actually work
  • Update the integrity hashes to match the new URLs
  • Make sure the fall back URLs and the CDN URLs have the same content

Next time you update, I would recommend adding a fall back URL on your blob storage. It has saved us a few times in the past when the CDN started misbehaving...

Copy link
Contributor

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

return value.replace(regex, newValue);
}

function verifyMatchCount(value: string, shouldContainVersion = false): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If I didn't write the code, I am not sure I would understand what "verifyMatchCount" does without reading the code. How about "verifyVersionSubstringCount"?

@NTaylorMullen
Copy link

Next time you update, I would recommend adding a fall back URL on your blob storage. It has saved us a few times in the past when the CDN started misbehaving...

Definitely on our plate for the next release 😄 https://github.com/aspnet/Razor.VSCode/issues/257

@akshita31 akshita31 merged commit 265499a into dotnet:master Jan 7, 2019
@akshita31 akshita31 deleted the omnisharp_update branch January 7, 2019 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants