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

Implemented getVersion in OpenShiftConnector #6222

Merged
merged 2 commits into from
Oct 2, 2017

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented Sep 11, 2017

Signed-off-by: jpinkney josh.pinkney@mail.utoronto.ca

What does this PR do?

Rebases #4363 onto master branch.

What issues does this PR fix or reference?

Rebase of: #4363
Part of: redhat-developer/rh-che#322

Changelog

Implemented getVersion in OpenShiftConnector

Release Notes

N/A

Docs PR

@codenvy-ci
Copy link

Can one of the admins verify this patch?

1 similar comment
@codenvy-ci
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

Thank you @JPinkney. I know that you haven't written the original code for this PR (that was a rebase of a PR that @snjeza did a few months ago) but it would be helpful if you could answer my comments.

openShiftClient.adapt(OkHttpClient.class),
OpenShiftConfig.wrap(openShiftClient.getConfiguration()));
String versionString = client.getVersion();
if (versionString == null || versionString.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use method isNullOrEmpty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and updated.

@@ -266,6 +271,25 @@ public void onEvent(ServerIdleEvent event) {
});
}

@Override
public Version getVersion() throws IOException {
@SuppressWarnings("resource")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need SuppressWarnings? Can't we avoid it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like it wasn't needed. I've removed it and updated the code.

@@ -0,0 +1,44 @@
/**
* ***************************************************************************** Copyright (c)
Copy link
Contributor

Choose a reason for hiding this comment

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

headers seem broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed header's in OpenShiftClientExtension and OpenShiftVersion.

@benoitf
Copy link
Contributor

benoitf commented Sep 13, 2017

ci-build

@benoitf benoitf added kind/task Internal things, technical debt, and to-do tasks to be performed. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. team/osio labels Sep 13, 2017
@codenvy-ci
Copy link

@l0rd
Copy link
Contributor

l0rd commented Sep 29, 2017

@JPinkney sorry you need to rebase this PR 😞

@codenvy-ci
Copy link

@benoitf benoitf merged commit 34ba429 into eclipse-che:master Oct 2, 2017
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Oct 2, 2017
@benoitf benoitf added this to the 5.19.0 milestone Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants