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

Download Helm from redirect url. #7805

Merged
merged 5 commits into from
Jul 26, 2018
Merged

Download Helm from redirect url. #7805

merged 5 commits into from
Jul 26, 2018

Conversation

vithati
Copy link
Member

@vithati vithati commented Jul 23, 2018

No description provided.

@vithati vithati requested review from Anumita and bansalaseem July 23, 2018 10:14
if (typeof options === 'string') {
options = redirectUrl;
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor this to a separate method call getRedirectOptions

import * as tl from "vsts-task-lib/task";

export async function download(url: any, downloadPath: string, printData: boolean): Promise<void> {
export async function download(options: any, downloadPath: string, printData: boolean, isRedirectUrl: boolean = false): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename isRedirectUrl this to handleRedirect

reject(new Error('Failed to download file status code: ' + res.statusCode));
if(res.statusCode < 200 || res.statusCode >= 300) {
if((res.statusCode >= 300 || res.statusCode < 400)
&& !isRedirectUrl
Copy link
Contributor

Choose a reason for hiding this comment

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

refactor this to isRedirect

}

function getRedirectOptions(options: any, redirectUrl: string): any {
tl.debug("Download latest release from redirect uri: " + redirectUrl);
Copy link
Contributor

@bansalaseem bansalaseem Jul 26, 2018

Choose a reason for hiding this comment

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

Change the debug log from Download latest release … to something more meaningful. This method should not know that it is being used in download function

Copy link
Contributor

@bansalaseem bansalaseem left a comment

Choose a reason for hiding this comment

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

Good looks very simple now. Thanks for making this change

@vithati vithati merged commit e66879a into master Jul 26, 2018
@vithati vithati deleted the users/vithati/helminstall branch July 26, 2018 13:43
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