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.

Sign up for GitHub

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

Jump to bottom

Fix #1139 : Make it easy to get the URL of a service. #1171

Merged
merged 1 commit into from
Aug 21, 2018
Merged

Fix #1139 : Make it easy to get the URL of a service. #1171

merged 1 commit into from
Aug 21, 2018

Conversation

rohanKanojia
Copy link
Member

#1139

  • Implemented getUrl() method for service
  • Fixed an arquillian dependency error that comes up while
    running tests in IntelliJ

@rohanKanojia rohanKanojia added the needs code review Pull requests which are waiting for review label Aug 3, 2018
lordofthejars
lordofthejars requested changes Aug 6, 2018
View reviewed changes
kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/URLFromServiceImplUtil.java Outdated
import java.util.Map;
import java.util.Objects;

public class URLFromServiceImplUtil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I will name it URLFromServiceUtil

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/URLFromServiceImplUtil.java Outdated
private static final String PORT_SUFFIX = "_SERVICE_PORT";
private static final String PROTO_SUFFIX = "_TCP_PROTO";

public static String serviceToHostOrBlank(String serviceName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I will call it resolveHostFromEnvVarOrSystemProperty and the same for port, proto, .... because when I read the code that calls this method I was not sure if it were resolving the env varor not.

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/URLFromNodePortImpl.java Outdated
NodeStatus status = item.getStatus();
if(!bFound && status != null) {
List<NodeAddress> addresses = status.getAddresses();
for(NodeAddress address : addresses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this second for inside a method? Just do not create two nested for directly. I usually try to avoid this since for a reader is easier to check what this method exactly do. Since Java does not have multi return like go, this new method could return a simple private class with required arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I wish we had already upgraded this project to Java 8, I could have used javafx.util.Pair 😄

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/URLFromNodePortImpl.java Outdated
if (!ip.isEmpty()) {
clusterIP = ip;
portNumber = nodePort;
bFound = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Following my previous comment bFound is not required anymore and you can just check for null.

...es-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/ServiceOperationsImpl.java
}

// Sort all loaded implementations according to priority
Collections.sort(servicesList, new ServiceToUrlSortComparator());
Copy link
Contributor

Choose a reason for hiding this comment

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

I will add from line 90 to 99 into a method that gets and order services, so the method getURL looks more compact.

iocanel
iocanel requested changes Aug 7, 2018
View reviewed changes
Copy link
Member

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

Overall, it looks good. The only thing I'd like to change is the priority enum, as I'd like to make room for extension and custom impls. So I'd use just an int. Our impls could use numbers like 10,20,30 or even 100,200,300.

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/ServiceToURLProvider.java Outdated
}

ServiceToUrlImplPriority getPriority();

Copy link
Member

Choose a reason for hiding this comment

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

I'd use an int instead of an enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh true well the enum can still be used but this method should return an int, so we can use our enum internally so calling .getValue() in implementations but it is true that the return type should be an int.

gastaldi
gastaldi reviewed Aug 10, 2018
View reviewed changes
kubernetes-client/src/main/java/io/fabric8/kubernetes/client/ServiceToURLProvider.java
import io.fabric8.kubernetes.api.model.Service;

public interface ServiceToURLProvider {
enum ServiceToUrlImplPriority {
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that priority is an int, is this enum really necessary?

@rohanKanojia
Copy link
Member Author

@iocanel : Hey, I updated according to your comments. Could you please take a look again whenever you get time :-)?

@rohanKanojia rohanKanojia requested a review from oscerd August 20, 2018 07:28
@rohanKanojia
Copy link
Member Author

@iocanel : polite ping, could you please take a look again?

@rohanKanojia
Copy link
Member Author

[merge]

@rohanKanojia
Copy link
Member Author

retest this please

@rohanKanojia
Copy link
Member Author

[merge]

@rohanKanojia
Copy link
Member Author

retest this please

@rohanKanojia
Copy link
Member Author

@iocanel @oscerd : Are builds cached in test job and not in merge job? Somehow it seems to be failing on fresh builds.

gastaldi
gastaldi reviewed Aug 20, 2018
View reviewed changes
kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/URLUtils.java Outdated
} catch (URISyntaxException exception) {
return false;
}
return url.contains("null") ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

url may be null at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, yeah. Thanks for pointing out

@rohanKanojia
Fix fabric8io#1139 : Make it easy to get the URL of a service.
7d8213d 
+ Implemented getUrl() method for service
+ Fixed an arquillian dependency error that comes up while
  running tests in IntelliJ
@rohanKanojia
Copy link
Member Author

[merge]

@fusesource-ci fusesource-ci merged commit aa86006 into fabric8io:master Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi left review comments

@iocanel iocanel iocanel requested changes

@oscerd oscerd oscerd approved these changes

@lordofthejars lordofthejars lordofthejars approved these changes

Assignees
No one assigned
Labels
needs code review Pull requests which are waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rohanKanojia @gastaldi @iocanel @lordofthejars @oscerd @fusesource-ci