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/14 22:32:20 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #4692: Deliveryservice Topologies
rawlinp opened a new pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692
## What does this PR (Pull Request) do?
Add a new `Topology` field to Delivery Services in TO API 3.0 in order to assign Delivery Services to Topologies.
- [x] This PR fixes #4569
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Control Client (Go)
- Traffic Ops
- Traffic Portal
## What is the best way to verify this PR?
1. Run the unit tests, verify they pass
2. Run the TO API tests, verify they pass
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] This PR includes documentation
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426795940
##########
File path: traffic_ops/testing/api/v3/user_test.go
##########
@@ -30,7 +30,7 @@ import (
)
func TestUsers(t *testing.T) {
- WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, DeliveryServices, Users}, func() {
+ WithObjs(t, []TCObj{Tenants, Users}, func() {
Review comment:
Ok that is the SMTP one...lemme turn on the external tests and look into that. Guessing maybe there is a data dependency I removed that is causing this to fail.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r430615974
##########
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:
yes, i would have but then i could. not use the `::` one way binding syntax.
##########
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:
yes, i would have but then i could not use the `::` one way binding syntax.
##########
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:
yeah, should probably be an ngHref for all of the `View Details` links across all of the forms but i think i'll create a tech debt global issue for that.
##########
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:
https://github.com/apache/trafficcontrol/issues/4719
##########
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:
fair enough although i'm pretty sure no type coercion will be occurring but, yeah, better practice.
##########
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:
`::! deliveryService.topology` nope
##########
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:
`::!deliveryService.topology` nope
##########
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:
actually, looks like this works. testing now. `ng-if="::(!deliveryService.topology)"`
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r430636305
##########
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:
really? That doesn't work?
##########
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:
nice
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html
##########
@@ -51,7 +51,7 @@
<hr class="divider"/>
<li role="menuitem"><a ng-click="viewCharts()">View Charts</a></li>
<hr class="divider"/>
- <li role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
+ <li ng-if="::(!deliveryService.topology)" role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
Review comment:
Nah
```javascript
const a = {topology: "test"};
console.log(a.topology instanceof Object); // false
```
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html
##########
@@ -51,7 +51,7 @@
<hr class="divider"/>
<li role="menuitem"><a ng-click="viewCharts()">View Charts</a></li>
<hr class="divider"/>
- <li role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
+ <li ng-if="::(!deliveryService.topology)" role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
Review comment:
But then you could have
```javascript
const a = {topology: ""};
console.log(typeof a.topology === "string"); //true
```
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] ocket8888 merged pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426898413
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests_test.go
##########
@@ -58,37 +58,29 @@ func TestGetDeliveryServiceRequest(t *testing.T) {
b := true
u := "UPDATE"
st := tc.RequestStatusSubmitted
+ ds := tc.DeliveryServiceNullable{}
+ ds.XMLID = &s
+ ds.CDNID = &i
+ ds.LogsEnabled = &b
+ ds.DSCP = nil
+ ds.GeoLimit = &i
+ ds.Active = &b
+ ds.TypeID = &i
Review comment:
Yeah, anywhere we're using anonymous embedded structs due to API versioning, it is most advantageous to do assignments this way (unfortunately).
##########
File path: docs/source/api/v3/deliveryservices.rst
##########
@@ -44,6 +44,8 @@ Request Structure
+--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+
| tenant | no | Show only the :term:`Delivery Services` belonging to the :term:`Tenant` identified by this integral, unique identifier |
+--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+
+ | topology | no | Show only the :term:`Delivery Services` assigned to the :term:`Topology` identified by this unique identifier |
Review comment:
fixed in 277c97e1d5d2490d4d07425dd3f6c3f5559bf0e6
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests_test.go
##########
@@ -58,37 +58,29 @@ func TestGetDeliveryServiceRequest(t *testing.T) {
b := true
u := "UPDATE"
st := tc.RequestStatusSubmitted
+ ds := tc.DeliveryServiceNullable{}
+ ds.XMLID = &s
+ ds.CDNID = &i
+ ds.LogsEnabled = &b
+ ds.DSCP = nil
+ ds.GeoLimit = &i
+ ds.Active = &b
+ ds.TypeID = &i
r := &TODeliveryServiceRequest{DeliveryServiceRequestNullable: tc.DeliveryServiceRequestNullable{
- ChangeType: &u,
- Status: &st,
- DeliveryService: &tc.DeliveryServiceNullable{
- DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
- DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
- DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
- DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
- XMLID: &s,
- CDNID: &i,
- LogsEnabled: &b,
- DSCP: nil,
- GeoLimit: &i,
- Active: &b,
- TypeID: &i,
- },
- },
- },
- },
- },
+ ChangeType: &u,
+ Status: &st,
+ DeliveryService: &ds,
}}
expectedErrors := []string{
- /*
- `'regionalGeoBlocking' is required`,
- `'xmlId' cannot contain spaces`,
- `'dscp' is required`,
- `'displayName' cannot be blank`,
- `'geoProvider' is required`,
- `'typeId' is required`,
- */
+ /*
+ `'regionalGeoBlocking' is required`,
+ `'xmlId' cannot contain spaces`,
+ `'dscp' is required`,
+ `'displayName' cannot be blank`,
+ `'geoProvider' is required`,
+ `'typeId' is required`,
+ */
Review comment:
fixed in 0e656a31fa08ca832bc0eedd6c51929742e1da51
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -906,6 +991,7 @@ func readGetDeliveryServices(params map[string]string, tx *sqlx.Tx, user *auth.C
"logsEnabled": dbhelpers.WhereColumnInfo{"ds.logs_enabled", api.IsBool},
"tenant": dbhelpers.WhereColumnInfo{"ds.tenant_id", api.IsInt},
"signingAlgorithm": dbhelpers.WhereColumnInfo{"ds.signing_algorithm", nil},
+ "topology": dbhelpers.WhereColumnInfo{"ds.topology", nil},
Review comment:
changed in f5809af714236285d8c8160930f78a640cb03d68 although I generally try to keep formatting-type changes unrelated to the original PR in separate PRs -- cuts down on the noise that way
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r430717460
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html
##########
@@ -51,7 +51,7 @@
<hr class="divider"/>
<li role="menuitem"><a ng-click="viewCharts()">View Charts</a></li>
<hr class="divider"/>
- <li role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
+ <li ng-if="::(!deliveryService.topology)" role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
Review comment:
IMO these should be !(deliveryservice.Topology instanceof Object)
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html
##########
@@ -51,7 +51,7 @@
<hr class="divider"/>
<li role="menuitem"><a ng-click="viewCharts()">View Charts</a></li>
<hr class="divider"/>
- <li role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
+ <li ng-if="::(!deliveryService.topology)" role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
Review comment:
Oh, that's just the name. `typeof deliveryService/Topology !== 'string'` in that case
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html
##########
@@ -51,7 +51,7 @@
<hr class="divider"/>
<li role="menuitem"><a ng-click="viewCharts()">View Charts</a></li>
<hr class="divider"/>
- <li role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
+ <li ng-if="::(!deliveryService.topology)" role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
Review comment:
Oh, that's just the name. `typeof deliveryService.Topology !== 'string'` in that case
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html
##########
@@ -51,7 +51,7 @@
<hr class="divider"/>
<li role="menuitem"><a ng-click="viewCharts()">View Charts</a></li>
<hr class="divider"/>
- <li role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
+ <li ng-if="::(!deliveryService.topology)" role="menuitem"><a ng-click="viewServers()">Manage Servers</a></li>
Review comment:
True, the loose comparison is useful in this case.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426844370
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests_test.go
##########
@@ -58,37 +58,29 @@ func TestGetDeliveryServiceRequest(t *testing.T) {
b := true
u := "UPDATE"
st := tc.RequestStatusSubmitted
+ ds := tc.DeliveryServiceNullable{}
+ ds.XMLID = &s
+ ds.CDNID = &i
+ ds.LogsEnabled = &b
+ ds.DSCP = nil
+ ds.GeoLimit = &i
+ ds.Active = &b
+ ds.TypeID = &i
Review comment:
That is true. In this case, multiple assignments results in less future effort relating to versioned DeliveryService structs.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426840702
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests_test.go
##########
@@ -58,37 +58,29 @@ func TestGetDeliveryServiceRequest(t *testing.T) {
b := true
u := "UPDATE"
st := tc.RequestStatusSubmitted
+ ds := tc.DeliveryServiceNullable{}
+ ds.XMLID = &s
+ ds.CDNID = &i
+ ds.LogsEnabled = &b
+ ds.DSCP = nil
+ ds.GeoLimit = &i
+ ds.Active = &b
+ ds.TypeID = &i
Review comment:
Multiple assignments have the advantage of continuing to work if the fields are changed to be an embedded anonymous struct.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426840702
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests_test.go
##########
@@ -58,37 +58,29 @@ func TestGetDeliveryServiceRequest(t *testing.T) {
b := true
u := "UPDATE"
st := tc.RequestStatusSubmitted
+ ds := tc.DeliveryServiceNullable{}
+ ds.XMLID = &s
+ ds.CDNID = &i
+ ds.LogsEnabled = &b
+ ds.DSCP = nil
+ ds.GeoLimit = &i
+ ds.Active = &b
+ ds.TypeID = &i
Review comment:
Multiple assignments have the advantage of continuing to work if the fields are changed to be a different embedded anonymous struct, or to not be one.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426824425
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -906,6 +991,7 @@ func readGetDeliveryServices(params map[string]string, tx *sqlx.Tx, user *auth.C
"logsEnabled": dbhelpers.WhereColumnInfo{"ds.logs_enabled", api.IsBool},
"tenant": dbhelpers.WhereColumnInfo{"ds.tenant_id", api.IsInt},
"signingAlgorithm": dbhelpers.WhereColumnInfo{"ds.signing_algorithm", nil},
+ "topology": dbhelpers.WhereColumnInfo{"ds.topology", nil},
Review comment:
This can be just
```go
"topology": {"ds.topology", nil},
```
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests_test.go
##########
@@ -58,37 +58,29 @@ func TestGetDeliveryServiceRequest(t *testing.T) {
b := true
u := "UPDATE"
st := tc.RequestStatusSubmitted
+ ds := tc.DeliveryServiceNullable{}
+ ds.XMLID = &s
+ ds.CDNID = &i
+ ds.LogsEnabled = &b
+ ds.DSCP = nil
+ ds.GeoLimit = &i
+ ds.Active = &b
+ ds.TypeID = &i
Review comment:
Instead of multiple assignments, this could be
```go
ds.DeliveryServiceNullableV11 = tc.DeliveryServiceNullableV11{/*the fields*/}
```
Not a definite improvement, but an option
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests_test.go
##########
@@ -58,37 +58,29 @@ func TestGetDeliveryServiceRequest(t *testing.T) {
b := true
u := "UPDATE"
st := tc.RequestStatusSubmitted
+ ds := tc.DeliveryServiceNullable{}
+ ds.XMLID = &s
+ ds.CDNID = &i
+ ds.LogsEnabled = &b
+ ds.DSCP = nil
+ ds.GeoLimit = &i
+ ds.Active = &b
+ ds.TypeID = &i
r := &TODeliveryServiceRequest{DeliveryServiceRequestNullable: tc.DeliveryServiceRequestNullable{
- ChangeType: &u,
- Status: &st,
- DeliveryService: &tc.DeliveryServiceNullable{
- DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
- DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
- DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
- DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
- XMLID: &s,
- CDNID: &i,
- LogsEnabled: &b,
- DSCP: nil,
- GeoLimit: &i,
- Active: &b,
- TypeID: &i,
- },
- },
- },
- },
- },
+ ChangeType: &u,
+ Status: &st,
+ DeliveryService: &ds,
}}
expectedErrors := []string{
- /*
- `'regionalGeoBlocking' is required`,
- `'xmlId' cannot contain spaces`,
- `'dscp' is required`,
- `'displayName' cannot be blank`,
- `'geoProvider' is required`,
- `'typeId' is required`,
- */
+ /*
+ `'regionalGeoBlocking' is required`,
+ `'xmlId' cannot contain spaces`,
+ `'dscp' is required`,
+ `'displayName' cannot be blank`,
+ `'geoProvider' is required`,
+ `'typeId' is required`,
+ */
Review comment:
`gofmt` re-indents this
##########
File path: docs/source/api/v3/deliveryservices.rst
##########
@@ -44,6 +44,8 @@ Request Structure
+--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+
| tenant | no | Show only the :term:`Delivery Services` belonging to the :term:`Tenant` identified by this integral, unique identifier |
+--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+
+ | topology | no | Show only the :term:`Delivery Services` assigned to the :term:`Topology` identified by this unique identifier |
Review comment:
IMO, "identified by this unique identifier" is ambiguous because I don't know if it's a string or a number or what additional meaning it has. More specifically, it is the topology's name. Calling it a unique name might provide clarity.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426785516
##########
File path: traffic_ops/testing/api/v3/user_test.go
##########
@@ -30,7 +30,7 @@ import (
)
func TestUsers(t *testing.T) {
- WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, DeliveryServices, Users}, func() {
+ WithObjs(t, []TCObj{Tenants, Users}, func() {
Review comment:
This API test is failing for me.
```shell
[zrhoffman@computer api]$ time go test './v3' -test.v -cfg=$PWD/conf/traffic-ops-test.conf -fixtures=$PWD/v3/tc-fixtures.json -test.run 'TestMain|TestUsers'
INFO: traffic_ops_test.go:71: 2020-05-18T10:58:09.505714571-06:00: Using Config values:
TO Config File: /home/zhoffm468/go/src/github.com/apache/trafficcontrol/traffic_ops/testing/api/conf/traffic-ops-test.conf
TO Fixtures: /home/zhoffm468/go/src/github.com/apache/trafficcontrol/traffic_ops/testing/api/v3/tc-fixtures.json
TO URL: https://localhost:6443
TO Session Timeout In Secs: 60
DB Server: localhost
DB User: traffic_ops
DB Name: traffic_ops
DB Ssl: false
=== RUN TestUsers
TestUsers: user_test.go:58: Response: {[{user was created. success}]}
TestUsers: user_test.go:58: Response: {[{user was created. success}]}
TestUsers: user_test.go:58: Response: {[{user was created. success}]}
TestUsers: user_test.go:58: Response: {[{user was created. success}]}
TestUsers: user_test.go:58: Response: {[{user was created. success}]}
TestUsers: user_test.go:58: Response: {[{user was created. success}]}
TestUsers: user_test.go:58: Response: {[{user was created. success}]}
TestUsers: user_test.go:155: could not register user: 500 Internal Server Error[500] - Error requesting Traffic Ops https://localhost:6443/api/3.0/users/register {"alerts":[{"text":"Internal Server Error","level":"error"}]}
--- FAIL: TestUsers (2.03s)
FAIL
FAIL github.com/apache/trafficcontrol/traffic_ops/testing/api/v3 2.320s
FAIL
real 0m3.824s
user 0m3.856s
sys 0m0.523s
```
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4692:
URL: https://github.com/apache/trafficcontrol/pull/4692#discussion_r426809077
##########
File path: traffic_ops/testing/api/v3/user_test.go
##########
@@ -30,7 +30,7 @@ import (
)
func TestUsers(t *testing.T) {
- WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, DeliveryServices, Users}, func() {
+ WithObjs(t, []TCObj{Tenants, Users}, func() {
Review comment:
Ok should be resolved now in https://github.com/apache/trafficcontrol/pull/4692/commits/46d204d80fb8644fb4345d7ddc504714ab67dd66
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4692: Deliveryservice Topologies
Posted by GitBox <gi...@apache.org>.
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