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

[DotnetCoreInstaller] Fetching download urls from releases.json file #7434

Merged
merged 11 commits into from
Jun 22, 2018

Conversation

thesattiraju
Copy link
Contributor

@thesattiraju thesattiraju commented Jun 12, 2018

@thesattiraju thesattiraju changed the title [DotnetCoreInstaller] Fetching download urls from releases.csv file [DotnetCoreInstaller] Fetching download urls from releases.json file Jun 19, 2018
Copy link
Member

@sachinma sachinma left a comment

Choose a reason for hiding this comment

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

Add UTs

@@ -36,7 +39,7 @@
"label": "Version",
"defaultValue": "1.0.4",
"required": true,
"helpMarkDown": "Specify exact version of .NET Core SDK or runtime to install.<br/><br/>Examples:<br/>1. To install 1.0.4 SDK, use 1.0.4<br/>2. To install 1.1.2 runtime, use 1.1.2<br/>2. To install 2.0 preview 2 runtime, use 2.0.0-preview2-25407-01<br/><br/>For getting more details about exact version, refer [here](https://github.com/dotnet/core/blob/master/release-notes/releases.csv)"
"helpMarkDown": "Specify exact version of .NET Core SDK or runtime to install.<br/><br/>Examples:<br/>1. To install 1.0.4 SDK, use 1.0.4<br/>2. To install 1.1.2 runtime, use 1.1.2<br/>2. To install 2.0 preview 2 runtime, use 2.0.0-preview2-25407-01<br/><br/>For getting more details about exact version of runtime (, refer [here](https://github.com/dotnet/core/blob/master/release-notes/releases.json)"
Copy link
Member

Choose a reason for hiding this comment

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

This is not precise. Fix this and work with atul if required.


export class DotNetCoreReleaseFetcher {

public static async getDownloadUrls(osSuffixes: string[], version: string, type: string) {
Copy link
Member

Choose a reason for hiding this comment

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

no return type

public static async getDownloadUrls(osSuffixes: string[], version: string, type: string) {
let downloadUrls = [];
let releasesCSV = await this.getReleasesJson();
let versionsInfo = JSON.parse(await releasesCSV.readBody());
Copy link
Member

Choose a reason for hiding this comment

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

why is the variable name csv


export class DotNetCoreReleaseFetcher {

public static async getDownloadUrls(osSuffixes: string[], version: string, type: string) {
Copy link
Member

Choose a reason for hiding this comment

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

add some comments explaining the parameters

let release = releasesInfo[0];
let blobUrl: string = release['blob-' + type];
let dlcUrl: string = release['dlc-' + type];
let fileName: string = release[type + '-' + osSuffixes[0]] ? release[type + '-' + osSuffixes[0]] : release[type + '-' + osSuffixes[1]];
Copy link
Member

Choose a reason for hiding this comment

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

you can check that the osSuffixes has a length of 2.

let output: string = result.stdout;

let index;
if (index = output.indexOf("Primary:")) {
Copy link
Member

Choose a reason for hiding this comment

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

This wont work since the index would be -1

command = command.concat(" -SharedRuntime");
try {
downloadPath = await toolLib.downloadTool(downloadUrls[i]);
downloaded = true;
Copy link
Member

Choose a reason for hiding this comment

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

break here itself instead of having an if condition at the top

@@ -6,7 +6,7 @@
"loc.input.label.packageType": "Package to install",
"loc.input.help.packageType": "Please select whether to install only runtime or full SDK.",
"loc.input.label.version": "Version",
"loc.input.help.version": "Specify exact version of .NET Core SDK or runtime to install.<br/><br/>Examples:<br/>1. To install 1.0.4 SDK, use 1.0.4<br/>2. To install 1.1.2 runtime, use 1.1.2<br/>2. To install 2.0 preview 2 runtime, use 2.0.0-preview2-25407-01<br/><br/>For getting more details about exact version of runtime (, refer [here](https://github.com/dotnet/core/blob/master/release-notes/releases.json)",
"loc.input.help.version": "Specify exact version of .NET Core SDK or runtime to install.<br/><br/>Examples:<br/>1. To install 1.0.4 SDK, use 1.0.4<br/>2. To install 1.1.2 runtime, use 1.1.2<br/>2. To install 2.0 preview 2 runtime, use 2.0.0-preview2-25407-01<br/><br/>For getting more details about exact version refer [here](https://github.com/dotnet/core/blob/master/release-notes/releases.json)",
Copy link
Member

Choose a reason for hiding this comment

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

The help text is not up-to the mark. Can you specify the exact strings to look for in the json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants