You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/05/21 20:56:35 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4692: Deliveryservice Topologies
ocket8888 commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r428907858
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
##########
@@ -228,6 +228,22 @@ <h3>Current Value</h3>
</aside>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error': hasError(generalConfig.topology), 'has-feedback': hasError(generalConfig.topology)}">
+ <label class="has-tooltip control-label col-md-2 col-sm-2 col-xs-12" for="topology">Topology<div class="helptooltip">
+ <div class="helptext">A topology is used to determine the caching layers and path to a delivery service's origin.</div>
+ </div>
+ </label>
+ <div class="col-md-10 col-sm-10 col-xs-12">
+ <select id="topology" name="topology" class="form-control" ng-model="deliveryService.topology" ng-options="topology.name as topology.name for topology in topologies">
+ <option selected value="">None</option>
+ </select>
+ <small ng-show="deliveryService.topology"><a href="/#!/topologies/edit?name={{deliveryService.topology}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
Review comment:
If you're putting a data-bound value into an `href` you should be using [`ngHref`](https://docs.angularjs.org/api/ng/directive/ngHref#!). You'll probably also want the entire path to which it re-directs to be in the model, as that will allow the routing to change location without reloading the page - the docs have an example of all the different ways you can use it.
##########
File path: traffic_portal/app/src/common/api/ServerService.js
##########
@@ -140,7 +140,15 @@ var ServerService = function($http, locationUtils, messageModel, ENV) {
};
this.getDeliveryServiceServers = function(dsId) {
- return $http.get(ENV.api['root'] + 'deliveryservices/' + dsId + '/servers').then(
+ return $http.get(ENV.api['root'] + 'servers?dsId=' + dsId).then(
Review comment:
you could also pass this as a param instead of using direct query string construction, but it's less of a big deal since that's a number (we hope - JS be crazy)
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
##########
@@ -228,6 +228,22 @@ <h3>Current Value</h3>
</aside>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error': hasError(generalConfig.topology), 'has-feedback': hasError(generalConfig.topology)}">
+ <label class="has-tooltip control-label col-md-2 col-sm-2 col-xs-12" for="topology">Topology<div class="helptooltip">
+ <div class="helptext">A topology is used to determine the caching layers and path to a delivery service's origin.</div>
+ </div>
+ </label>
+ <div class="col-md-10 col-sm-10 col-xs-12">
+ <select id="topology" name="topology" class="form-control" ng-model="deliveryService.topology" ng-options="topology.name as topology.name for topology in topologies">
+ <option selected value="">None</option>
+ </select>
+ <small ng-show="deliveryService.topology"><a href="/#!/topologies/edit?name={{deliveryService.topology}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
+ <aside class="current-value" ng-if="settings.isRequest" ng-show="open() && deliveryService.topology != dsCurrent.topology">
Review comment:
Inequality comparisons should use `!==` instead of `!=`
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
##########
@@ -59,7 +59,7 @@
<li role="menuitem"><a ng-click="viewOrigins()">Manage Origins</a></li>
<li role="menuitem"><a ng-click="viewRegexes()">Manage Regexes</a></li>
<li role="menuitem"><a ng-click="viewCapabilities()">Manage Required Server Capabilities</a></li>
- <li role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
+ <li ng-if="::deliveryService.topology == null" role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
Review comment:
You should probably do `!deliveryService.topology` instead of using type-coerced null comparison - that way it'll still be disabled if that property is `undefined` or an empty string.
##########
File path: traffic_portal/app/src/common/api/ServerService.js
##########
@@ -140,7 +140,15 @@ var ServerService = function($http, locationUtils, messageModel, ENV) {
};
this.getDeliveryServiceServers = function(dsId) {
- return $http.get(ENV.api['root'] + 'deliveryservices/' + dsId + '/servers').then(
+ return $http.get(ENV.api['root'] + 'servers?dsId=' + dsId).then(
+ function (result) {
+ return result.data.response;
+ }
+ );
+ };
+
+ this.getTopologyServers = function(topology) {
+ return $http.get(ENV.api['root'] + 'servers?topology=' + topology).then(
Review comment:
that'll need to be url-encoded - which is easily accomplished by just passing it in the `params` instead of constructing a URL with a query string.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org