Skip to content

Proxy support for browserstack plugin #28

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

Merged
merged 18 commits into from
Nov 11, 2020

Conversation

harshitvasu
Copy link

@harshitvasu harshitvasu commented Nov 6, 2020

RELATED PR
https://github.com/browserstack/automate-client-java/pull/5/files
Jenkins plugin work behind proxy support (Automate client java changes)

Comment on lines 24 to 25
String proxyHost = jenkinsProxy.name;
int proxyPort = jenkinsProxy.port;
Copy link
Contributor

Choose a reason for hiding this comment

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

final for both...

Copy link
Contributor

Choose a reason for hiding this comment

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

can be resolved

Comment on lines 72 to 73
String username = JenkinsProxySettings.getUsername();
String password = JenkinsProxySettings.getPassword();
Copy link
Contributor

Choose a reason for hiding this comment

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

final .... consider final as const of JS. or any value which won't be re-assigned further...

Copy link
Contributor

Choose a reason for hiding this comment

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

can be resolved


public class JenkinsProxySettings {

static final ProxyConfiguration jenkinsProxy = Jenkins.getInstanceOrNull() != null ? Jenkins.getInstanceOrNull().proxy : null;

Choose a reason for hiding this comment

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

Suggested change
static final ProxyConfiguration jenkinsProxy = Jenkins.getInstanceOrNull() != null ? Jenkins.getInstanceOrNull().proxy : null;
private static final ProxyConfiguration jenkinsProxy = Jenkins.getInstanceOrNull() != null ? Jenkins.getInstanceOrNull().proxy : null;

Please define correct accessSpecifiers wherever necessary.

Copy link

@Archish27 Archish27 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rchougule rchougule left a comment

Choose a reason for hiding this comment

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

LGTM

@francisf francisf merged commit 3d39987 into jenkinsci:master Nov 11, 2020
@francisf francisf changed the title ACE-1568 Proxy support for browserstack plugin Proxy support for browserstack plugin Nov 11, 2020
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.

6 participants