Skip to content

[OSJC-184] Add port forwarding capability #40

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
Closed
wants to merge 1 commit into from
Closed

[OSJC-184] Add port forwarding capability #40

wants to merge 1 commit into from

Conversation

jcantrill
Copy link
Contributor

@adietish @maxandersen This is for your initial review. I'll push the JBT code that depends upon this. I need to do some more testing for cases like when it cant find the binary and also on windows.

@jcantrill jcantrill force-pushed the 184_port_forwarding branch 2 times, most recently from 18ba32e to 38baa62 Compare June 28, 2015 01:04
@adietish
Copy link
Member

looks fine to me. Like the capability visitor.

adietish
adietish reviewed Jun 29, 2015
View reviewed changes
...va/com/openshift/internal/restclient/capability/resources/OpenShiftBinaryPortForwarding.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I would extract the path building to a separate method, that would keep this method focused and enhance readability

@adietish
Copy link
Member

I tried to build my local origin so that I have the oc available, but it failed for me: http://fpaste.org/237582/55745821/
any idea what's wrong for me?

@adietish adietish mentioned this pull request Jun 29, 2015
Closed
@jcantrill jcantrill force-pushed the 184_port_forwarding branch from 38baa62 to 5b8bec5 Compare June 29, 2015 13:10
@jcantrill
Copy link
Contributor Author

@adietish try 'make clean; make'

@jcantrill
Copy link
Contributor Author

@adietish refactored and made more readable per your suggestions. Still needs testing on windows and mac

@adietish
Copy link
Member

@jcantrill ok that helped, thx!

@adietish
Copy link
Member

+1 good to go as soon as we did the basic testing on all platforms as discussed in irc.

@adietish
Copy link
Member

I have a test failure when running the tests via maven:

Tests in error: 
   getContainerPorts(com.openshift.internal.restclient.model.v1beta3.PodTest): No path was found for property 'ports.name' in the property map.

@jcantrill
Copy link
Contributor Author

Fixed test and repushed

@jcantrill jcantrill force-pushed the 184_port_forwarding branch 2 times, most recently from 977d613 to 8f6d9e6 Compare July 8, 2015 20:49
@jcantrill jcantrill force-pushed the 184_port_forwarding branch from 8f6d9e6 to 542d460 Compare July 9, 2015 01:45
@jcantrill
Copy link
Contributor Author

@adietish I have tested on linux and windows and am comfortable with merging this. Please review and comment accordingly.

@jcantrill
Copy link
Contributor Author

merged into master

@jcantrill jcantrill closed this Jul 9, 2015
@jcantrill jcantrill deleted the 184_port_forwarding branch July 9, 2015 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jcantrill @adietish