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/08/17 15:19:13 UTC
[GitHub] [trafficcontrol] zrhoffman opened a new pull request #4966: TO/TP: View servers by topology-based delivery service
zrhoffman opened a new pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
- [x] This PR fixes #4965 <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
- [x] This PR fixes #4964 <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
This PR
- lets you query servers by delivery service ID when the delivery service is topology-based
- lets you view servers for topology-based DSes in Traffic Portal
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- Traffic Ops
- Traffic Portal
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
- Run TO API tests
- Run TP UI tests
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [ ] This PR includes tests
- [ ] This PR includes documentation
- [ ] 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)
## Additional Information
<!-- If you would like to include any additional information on the PR for
potential reviewers please put it here.
Some examples of this would be:
- Before and after screenshots/gifs of the Traffic Portal if it is affected
- Links to other dependent Pull Requests
- References to relevant context (e.g. new/updates to dependent libraries,
mailing list records, blueprints)
Feel free to leave this section blank (or, preferably, delete it entirely).
-->
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r478951846
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -166,24 +166,54 @@ func GetTestServersQueryParameters(t *testing.T) {
params := url.Values{}
params.Add("dsId", strconv.Itoa(*ds.ID))
- _, _, err = TOSession.GetServers(¶ms)
+ _, _, err = TOSession.GetServersWithHdr(¶ms, nil)
if err != nil {
t.Fatalf("Failed to get server by Delivery Service ID: %v", err)
}
currentTime := time.Now().UTC().Add(5 * time.Second)
- time := currentTime.Format(time.RFC1123)
+ timestamp := currentTime.Format(time.RFC1123)
var header http.Header
header = make(map[string][]string)
- header.Set(rfc.IfModifiedSince, time)
+ header.Set(rfc.IfModifiedSince, timestamp)
_, reqInf, _ := TOSession.GetServersWithHdr(¶ms, header)
if reqInf.StatusCode != http.StatusNotModified {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds = range dses {
+ if ds.XMLID != nil && *ds.XMLID != topDsXmlId {
+ continue
+ }
+ foundTopDs = true
+ break
Review comment:
Oops, I just mindlessly added a nil check. Logic fixed in 488f88fb8
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r480385076
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
Review comment:
The space in
```rst
:term:` Topology`
```
is causing it to render improperly
----------------------------------------------------------------
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 pull request #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#issuecomment-679136839
> This should come with an update to the documentation of the `/servers` endpoint's GET handler.
`GET /servers` documentation updated in 73bd7166dc
> Also, this change affects all API versions - although not structurally. I'm not necessarily opposed to doing that, but it'll mean adding `.. versionchanged:: ATCv5.0` to the older versions' pages, as clients may expect based on experience that the returned list is mutable through DS-to-server assignment changes.
Made `dsId` work for topology-based DSes in only API version 3 and up in eef6fae3f9
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r478483186
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -650,10 +673,15 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
if userErr != nil || sysErr != nil {
return nil, 0, errors.New("Forbidden"), sysErr, http.StatusForbidden, nil
}
+
+ var joinSubQuery string
+ if version.Major >= 3 {
Review comment:
Added null checks to calling functions and changed the function signature to accept a non-pointer version in 50ae3819eb.
----------------------------------------------------------------
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 pull request #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#issuecomment-682404686
- Fixed an assertion in e25f3d3e56 that broke elsewhere in the servers tests from using the non-deprecated client functions in 4c2790ccce
- I don't know why it took 10 minutes to run the v3 API tests, but they pass now as of 156a2ef7a9 (changing a fixtures topology cachegroup)
----------------------------------------------------------------
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 pull request #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#issuecomment-685082319
Fixed considering the DS's required capabilities in 7751fc204b (for API >= v3 only)
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r480409247
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
Review comment:
Removed space in 348c20da2
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r478483186
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -650,10 +673,15 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
if userErr != nil || sysErr != nil {
return nil, 0, errors.New("Forbidden"), sysErr, http.StatusForbidden, nil
}
+
+ var joinSubQuery string
+ if version.Major >= 3 {
Review comment:
Added null checks to calling functions and changed the function signature to accept a non-pointer version in 229ec8a7f5.
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds := range dses {
+ if (*ds.XMLID != topDsXmlId) {
Review comment:
Added nil check in 7972c31e1
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds := range dses {
Review comment:
Unshadowed in 815407d27
##########
File path: docs/source/glossary.rst
##########
@@ -422,6 +422,14 @@ Glossary
Tenancies
Users are grouped into :dfn:`Tenants` (or :dfn:`Tenancies`) to segregate ownership of and permissions over :term:`Delivery Services` and their resources. To be clear, the notion of :dfn:`Tenancy` **only** applies within the context of :term:`Delivery Services` and does **not** apply permissions restrictions to any other aspect of Traffic Control.
+ Topology Node
+ Topology Nodes
+ Parent Topology Node
+ Parent Topology Nodes
+ Child Topology Node
+ Child Topology Nodes
+ Each :term:`Topology Node` is associated with a particular :term:`Cache Group`. In addition, the Topology Node has 0, 1, or 2 Parent Topology Nodes and has 0, 1, or 2 Child Topology Nodes, according to your configuration.
Review comment:
Using `:dfn:` role in bb3c3b25f0
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
+ | | no | each server whose :ref:`Cache Group` is associated with a :term:`Topology Node` of that Topology. |
Review comment:
Changed to `:term:` role in fd9d1309f7
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r482517408
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -650,10 +695,27 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
if userErr != nil || sysErr != nil {
return nil, 0, errors.New("Forbidden"), sysErr, http.StatusForbidden, nil
}
+
+ var joinSubQuery string
+ if version.Major >= 3 {
+ const hasRequiredCapabilitiesQuery = `
+SELECT EXISTS(
Review comment:
Moved the query to the `deliveryservice` package in 51446fa30, which should also fix code folding.
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r478482947
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
+ | | no | each server whose :ref:`Cache Group` is associated with a :term:`Topology Node` of that Topology. |
Review comment:
Changed to `:term:` role in 64fe080020
##########
File path: docs/source/glossary.rst
##########
@@ -422,6 +422,14 @@ Glossary
Tenancies
Users are grouped into :dfn:`Tenants` (or :dfn:`Tenancies`) to segregate ownership of and permissions over :term:`Delivery Services` and their resources. To be clear, the notion of :dfn:`Tenancy` **only** applies within the context of :term:`Delivery Services` and does **not** apply permissions restrictions to any other aspect of Traffic Control.
+ Topology Node
+ Topology Nodes
+ Parent Topology Node
+ Parent Topology Nodes
+ Child Topology Node
+ Child Topology Nodes
+ Each :term:`Topology Node` is associated with a particular :term:`Cache Group`. In addition, the Topology Node has 0, 1, or 2 Parent Topology Nodes and has 0, 1, or 2 Child Topology Nodes, according to your configuration.
Review comment:
Using `:dfn:` role in f4b99beebc
----------------------------------------------------------------
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 pull request #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#issuecomment-682119284
Merge conflicts fixed
----------------------------------------------------------------
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 pull request #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#issuecomment-675764661
Rebased to get the API tests GitHub action
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r481786719
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:`Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
Review comment:
Table is malformed because of misaligned <kbd>|</kbd>s on this line.
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -650,10 +695,27 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
if userErr != nil || sysErr != nil {
return nil, 0, errors.New("Forbidden"), sysErr, http.StatusForbidden, nil
}
+
+ var joinSubQuery string
+ if version.Major >= 3 {
+ const hasRequiredCapabilitiesQuery = `
+SELECT EXISTS(
Review comment:
Would you mind indenting this consistent with its containing block? It's not required by `go fmt` and it's fine syntactically as-is, but it breaks some text editors' code folding.
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r480409247
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
Review comment:
Removed space in d6812cd532
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r478916668
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -166,24 +166,54 @@ func GetTestServersQueryParameters(t *testing.T) {
params := url.Values{}
params.Add("dsId", strconv.Itoa(*ds.ID))
- _, _, err = TOSession.GetServers(¶ms)
+ _, _, err = TOSession.GetServersWithHdr(¶ms, nil)
if err != nil {
t.Fatalf("Failed to get server by Delivery Service ID: %v", err)
}
currentTime := time.Now().UTC().Add(5 * time.Second)
- time := currentTime.Format(time.RFC1123)
+ timestamp := currentTime.Format(time.RFC1123)
var header http.Header
header = make(map[string][]string)
- header.Set(rfc.IfModifiedSince, time)
+ header.Set(rfc.IfModifiedSince, timestamp)
_, reqInf, _ := TOSession.GetServersWithHdr(¶ms, header)
if reqInf.StatusCode != http.StatusNotModified {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds = range dses {
+ if ds.XMLID != nil && *ds.XMLID != topDsXmlId {
+ continue
+ }
+ foundTopDs = true
+ break
Review comment:
idk if this logic makes sense. So if you get a Delivery Service with a nil XMLID it's the `ds-top` DS?
----------------------------------------------------------------
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 pull request #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#issuecomment-679133739
Rebased again to get more CI stuff
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r476596808
##########
File path: docs/source/glossary.rst
##########
@@ -422,6 +422,14 @@ Glossary
Tenancies
Users are grouped into :dfn:`Tenants` (or :dfn:`Tenancies`) to segregate ownership of and permissions over :term:`Delivery Services` and their resources. To be clear, the notion of :dfn:`Tenancy` **only** applies within the context of :term:`Delivery Services` and does **not** apply permissions restrictions to any other aspect of Traffic Control.
+ Topology Node
+ Topology Nodes
+ Parent Topology Node
+ Parent Topology Nodes
+ Child Topology Node
+ Child Topology Nodes
+ Each :term:`Topology Node` is associated with a particular :term:`Cache Group`. In addition, the Topology Node has 0, 1, or 2 Parent Topology Nodes and has 0, 1, or 2 Child Topology Nodes, according to your configuration.
Review comment:
Don't link to a definition within itself - the defining instance of a term should use the `dfn` text role.
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
+ | | no | each server whose :ref:`Cache Group` is associated with a :term:`Topology Node` of that Topology. |
Review comment:
`undefined label: cache group (if the link has no caption the label must precede a section header)`
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds := range dses {
Review comment:
`ds` shadows a name from its containing scope
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -650,10 +673,15 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
if userErr != nil || sysErr != nil {
return nil, 0, errors.New("Forbidden"), sysErr, http.StatusForbidden, nil
}
+
+ var joinSubQuery string
+ if version.Major >= 3 {
Review comment:
should check that the version is non-null before dereferencing property to avoid segfault
also, though, I don't know why we decided `APIInfo.Version` should be a pointer value. Maybe for plugins? If I were you, I'd check it in the caller and make this function take just a regular `api.Version`. Hopefully at some point we can put that check into `api.NewInfo` and make it not give out possibly null API versions. Since if you can't tell what version someone requested, you already know they requested an invalid URI, so you can just error out right there.
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds := range dses {
+ if (*ds.XMLID != topDsXmlId) {
Review comment:
should check if the pointer is null before dereferencing to avoid segfault
----------------------------------------------------------------
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 pull request #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#issuecomment-675087748
Ready for review!
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r482510802
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:`Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
Review comment:
Aligned `|`s in 1175775c5a
----------------------------------------------------------------
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 #4966: TO/TP: View servers by topology-based delivery service
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r478482947
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
+============+==========+===================================================================================================================+
| cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` |
+------------+----------+-------------------------------------------------------------------------------------------------------------------+
- | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+ | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+ | | no | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return |
+ | | no | each server whose :ref:`Cache Group` is associated with a :term:`Topology Node` of that Topology. |
Review comment:
Changed to `:term:` role in 8d86b6dab8
##########
File path: docs/source/glossary.rst
##########
@@ -422,6 +422,14 @@ Glossary
Tenancies
Users are grouped into :dfn:`Tenants` (or :dfn:`Tenancies`) to segregate ownership of and permissions over :term:`Delivery Services` and their resources. To be clear, the notion of :dfn:`Tenancy` **only** applies within the context of :term:`Delivery Services` and does **not** apply permissions restrictions to any other aspect of Traffic Control.
+ Topology Node
+ Topology Nodes
+ Parent Topology Node
+ Parent Topology Nodes
+ Child Topology Node
+ Child Topology Nodes
+ Each :term:`Topology Node` is associated with a particular :term:`Cache Group`. In addition, the Topology Node has 0, 1, or 2 Parent Topology Nodes and has 0, 1, or 2 Child Topology Nodes, according to your configuration.
Review comment:
Using `:dfn:` role in b17101fc3e
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds := range dses {
Review comment:
Unshadowed in db1ac6c739
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
}
+ foundTopDs := false
+ const topDsXmlId = "ds-top"
+ for _, ds := range dses {
+ if (*ds.XMLID != topDsXmlId) {
Review comment:
Added nil check in 0c9ff12fa0
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -650,10 +673,15 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
if userErr != nil || sysErr != nil {
return nil, 0, errors.New("Forbidden"), sysErr, http.StatusForbidden, nil
}
+
+ var joinSubQuery string
+ if version.Major >= 3 {
Review comment:
Added null checks to calling functions and changed the function signature to accept a non-pointer version in da586b6805.
----------------------------------------------------------------
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