Skip to content

Conversation

maciaszczykm
Copy link
Member

Connected to discussion from pull request #320 and issue #313:

I am not fully happy with leaving the link empty on local cluster. What about putting "localhost:nodeport" into the link as a fallback if no IP is available? First, I would help us for writing click-tests, because using external link would make the test work on local cluster (CI) and real cluster. Second, the link is an important UI feature, it shouldn't be missing on local cluster

I've set localhost:targetport as a external endpoint for cluster IP services. I've used target port instead of nodeport, because it was always 0 during tests. Is it correct?

Please take a look and say your opinion about it. Change is not big, so I can quickly adapt it (for example use node port instead of target port).

CC @cheld @bryk

@codecov-io
Copy link

Current coverage is 79.86%

Merging #369 into master will not affect coverage as of 6b600b5

@@            master    #369   diff @@
======================================
  Files           72      72       
  Stmts          581     581       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit            464     464       
  Partial          0       0       
  Missed         117     117       

Review entire Coverage Diff as of 6b600b5

Powered by Codecov. Updated on successful CI builds.

@bryk
Copy link
Contributor

bryk commented Feb 12, 2016

I like the size and scope of this PR, btw. "one thing at a time".

@cheld
Copy link
Contributor

cheld commented Feb 15, 2016

I can't see a link...are you sure it is working?

@bryk
Copy link
Contributor

bryk commented Feb 15, 2016

Wasn't this supposed to return localhost addresses only for nodeports/loadbalancers that fail to resolve any other meaningful address. Am I right?

CC @cheld

@cheld
Copy link
Contributor

cheld commented Feb 15, 2016

Yes exactly..I will try it once again.

@maciaszczykm
Copy link
Member Author

Okay, so this need to be refactored. I'll let you know when it will be ready for review.

@maciaszczykm
Copy link
Member Author

Wasn't this supposed to return localhost addresses only for nodeports/loadbalancers that fail to resolve any other meaningful address. Am I right?

Yes exactly..I will try it once again.

@cheld @bryk Updated. PTAL

@cheld
Copy link
Contributor

cheld commented Feb 17, 2016

I tested for local cluster. It works exactly as intended. I have not tested on vagrant or gce, yet.

@cheld
Copy link
Contributor

cheld commented Feb 18, 2016

LGTM

@maciaszczykm
Copy link
Member Author

@bryk PTAL

@bryk
Copy link
Contributor

bryk commented Feb 18, 2016

LGTM :)

bryk added a commit that referenced this pull request Feb 18, 2016
Add external endpoint for cluster IP services
@bryk bryk merged commit 3ecce3f into kubernetes:master Feb 18, 2016
@maciaszczykm maciaszczykm deleted the localhost-endpoint branch February 18, 2016 13:22
anvithks pushed a commit to anvithks/k8s-dashboard that referenced this pull request Sep 27, 2021
Fixed kubernetes#366. Added check if AK/SK exists when user accesses multi-cloud functions
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.

5 participants