Link for external endpoints #399

cheld · 2016-02-19T07:22:59Z

In demos, I want to show the running application after deployment in an easy way. I think this is also a natural behavior for users to deploy and then check the running application.

Now, the user could copy & paste the text from the endpoint, but even this is not possible any longer as the text is potentially shortened.

To solve that, I created a http link for external endpoints. It is also indicated by the mock-ups.

After thinking about it for a while I realized that this might not be correct all the time. Nodeports could be anything, e.g. a Minecraft server - not necessarily a HTTP server. In case of GCE load-balancer I am not sure. I assume it is only intended for HTTP...

After contemplating again, I still think I should add such a link. The advantages outweight the disadvantages. Maybe we style it a little bit differently. I am tempted to just add an icon next to the link, like in the mockup.

Any opinion?

codecov-io · 2016-02-19T07:31:16Z

Current coverage is `81.52%`

Merging #399 into master will decrease coverage by -0.10% as of 60b86f7

@@            master    #399   diff @@
======================================
  Files           75      76     +1
  Stmts          615     617     +2
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            502     503     +1
  Partial          0       0       
- Missed         113     114     +1

Review entire Coverage Diff as of 60b86f7

Powered by Codecov. Updated on successful CI builds.

bryk · 2016-02-19T09:56:46Z

src/app/frontend/replicationcontrollerdetail/serviceendpoint_directive.js

@@ -22,6 +22,7 @@ export default function serviceEndpointDirective() {
    templateUrl: 'replicationcontrollerdetail/serviceendpoint.html',
    scope: {
      'endpoint': '=',
+      'url': '@',


When url param is '@' you'll show it as a link when the value is 'foo' 'true' 'false' or 'x'. How about changing it to =?

This parameter should have a more descriptive name. Url does not convey what it actually is. You wouldn't know that this is a boolean value to display the endpoint as a link. How about is-http or is-external-http or something else to say that this is an externally available http endpoint. Using this info we can display this as a link.

I was thinking about some heuristic that would actually establish if endpoint has something that is worth showing as a link. Checking common ports (HTTP/HTTPS) and maybe trying to actually check if this link points to something that browser can display.

Checking this as true in code would make link available for everything and this is not desired I guess.

What do you think?

Can you document what the param means? A short note would suffice.

@floreks I think this is not feasible. We would have to ping the server. This would be slow and might do harm to it.

bryk · 2016-02-19T10:08:24Z

We need open_in_new icon next to the endpoints. Like this:

bryk · 2016-02-19T10:09:33Z

src/app/frontend/replicationcontrollerlist/replicationcontrollercard.html

@@ -111,7 +111,7 @@
          <span flex="initial" class="kd-replicationcontroller-card-section-title">External endpoint</span>
          <div flex>
            <div ng-repeat="endpoint in ::ctrl.replicationController.externalEndpoints track by $index">
-              <kd-service-endpoint endpoint="::endpoint"></kd-service-endpoint>
+              <kd-service-endpoint endpoint="::endpoint" url="true"></kd-service-endpoint>


Add an implementation note here that says why we always show external endpoints as links. Basically the same as you did in the PR comment. Use <!-- .... --!>

bryk · 2016-02-19T10:10:55Z

This is really helpful. Small change that brings a lot. Thanks! :)

cheld · 2016-02-22T15:41:11Z

what about separating the directive "kd-service-endpoint" in two directives instead of the boolean flag? e.g.:

kd-external-endpoint
kd-internal-endpoint

PTAL

bryk · 2016-02-23T07:20:19Z

what about separating the directive "kd-service-endpoint" in two directives instead of the boolean flag? e.g.:

Yes, this makes a lot of sense. I expect that we'll write a more sophisticated logic to detect whether to link-ify an endpoint and kd-external-endpoint directive would be a place for this. Please do this :)

cheld · 2016-02-23T13:46:37Z

PTAL

floreks · 2016-02-23T13:48:54Z

src/app/frontend/replicationcontrollerdetail/internalendpoint.html

@@ -14,9 +14,10 @@
 limitations under the License.
 -->

+


Not needed here.

cheld · 2016-02-23T14:39:53Z

PTAL

bryk · 2016-02-23T14:46:35Z

PTAL, a few style comments.

Reviewed 6 of 9 files at r2, 1 of 1 files at r3.
Review status: 6 of 7 files reviewed at latest revision, 5 unresolved discussions.

src/app/frontend/replicationcontrollerdetail/externalendpoint_directive.js, line 20 [r3] (raw file):
Rename to externalEndpoint...

src/app/frontend/replicationcontrollerdetail/internalendpoint_directive.js, line 20 [r3] (raw file):
Rename to internalEndpoint...

Comments from the review on Reviewable.io

cheld · 2016-02-24T11:58:44Z

Thanks, PTAL

bryk · 2016-02-24T13:24:28Z

Reviewed 1 of 9 files at r2, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

Link for external endpoints

googlebot added the cla: yes label Feb 19, 2016

bryk reviewed Feb 19, 2016
View reviewed changes

cheld force-pushed the url-for-external-endpoint branch 3 times, most recently from 2344bdf to c413e90 Compare February 22, 2016 15:37

bryk added this to the v1.0 milestone Feb 23, 2016

cheld force-pushed the url-for-external-endpoint branch 3 times, most recently from 8d240bf to 66b8edf Compare February 23, 2016 13:36

floreks reviewed Feb 23, 2016
View reviewed changes

cheld force-pushed the url-for-external-endpoint branch from b6d9a4e to 799b886 Compare February 23, 2016 14:38

cheld force-pushed the url-for-external-endpoint branch from 799b886 to 82a1a1c Compare February 24, 2016 07:38

link for external endpoints

bd67629

cheld force-pushed the url-for-external-endpoint branch from 82a1a1c to bd67629 Compare February 24, 2016 11:56

bryk added a commit that referenced this pull request Feb 24, 2016

Merge pull request #399 from cheld/url-for-external-endpoint

a133e2d

Link for external endpoints

bryk merged commit a133e2d into kubernetes:master Feb 24, 2016

bryk deleted the url-for-external-endpoint branch February 24, 2016 13:24

maciaszczykm mentioned this pull request Mar 1, 2016

External Endpoint is abbreviated to useless #473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link for external endpoints #399

Link for external endpoints #399

cheld commented Feb 19, 2016

codecov-io commented Feb 19, 2016

bryk Feb 19, 2016

bryk Feb 19, 2016

floreks Feb 19, 2016

bryk Feb 19, 2016

cheld Feb 22, 2016

bryk commented Feb 19, 2016

bryk Feb 19, 2016

bryk commented Feb 19, 2016

cheld commented Feb 22, 2016

bryk commented Feb 23, 2016

cheld commented Feb 23, 2016

floreks Feb 23, 2016

cheld commented Feb 23, 2016

bryk commented Feb 23, 2016

cheld commented Feb 24, 2016

bryk commented Feb 24, 2016

Link for external endpoints #399

Link for external endpoints #399

Conversation

cheld commented Feb 19, 2016

codecov-io commented Feb 19, 2016

Current coverage is 81.52%

bryk Feb 19, 2016

Choose a reason for hiding this comment

bryk Feb 19, 2016

Choose a reason for hiding this comment

floreks Feb 19, 2016

Choose a reason for hiding this comment

bryk Feb 19, 2016

Choose a reason for hiding this comment

cheld Feb 22, 2016

Choose a reason for hiding this comment

bryk commented Feb 19, 2016

bryk Feb 19, 2016

Choose a reason for hiding this comment

bryk commented Feb 19, 2016

cheld commented Feb 22, 2016

bryk commented Feb 23, 2016

cheld commented Feb 23, 2016

floreks Feb 23, 2016

Choose a reason for hiding this comment

cheld commented Feb 23, 2016

bryk commented Feb 23, 2016

cheld commented Feb 24, 2016

bryk commented Feb 24, 2016

Current coverage is `81.52%`