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 2021/02/02 23:15:04 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5489: Non-"CRUD-er" DSRs
ocket8888 opened a new pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489
## What does this PR (Pull Request) do?
- [x] This PR fixes #5033 and #5015 and #5014 and #5012
This also, importantly, rewrites DSRs to not use the "CRUD-er", which will allow their structure to vary between API versions. This is necessary for upcoming changes to the endpoint which were previously included in the now-obsolete #5071 .
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Ops Go Client
- Traffic Ops
## What is the best way to verify this PR?
Run all associated tests, verify that the reproduction methods for each fixed bug no longer reproduce the respective bug.
## If this is a bug fix, what versions of Traffic Control are affected?
- master
- 5.0.0
- 4.1.1
## 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 FIX A SERIOUS SECURITY VULNERABILITY**
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-796274391
> I was able to get `orderby` and `sortOrder` to work. I even tried specifically ordering by `createdAt`. Worked fine.
yeah i can't get the `sortOrder` to stick when i do GET /api/4.0/deliveryservice_requests?orderby=createdAt&sortOrder=desc&limit=10 in your branch, i get this query:
```
SELECT
a.username AS author,
e.username AS lastEditedBy,
s.username AS assignee,
r.assignee_id,
r.author_id,
r.change_type,
r.created_at,
r.id,
r.last_edited_by_id,
r.last_updated,
r.deliveryservice,
r.status
FROM deliveryservice_request r
JOIN tm_user a ON r.author_id = a.id
LEFT OUTER JOIN tm_user s ON r.assignee_id = s.id
LEFT OUTER JOIN tm_user e ON r.last_edited_by_id = e.id
WHERE (
CAST(r.deliveryservice->>'tenantId' AS BIGINT) = ANY(CAST(:accessibleTenants AS BIGINT[]))
)
ORDER BY r.created_at
LIMIT 10
```
^^ see? no DESC attached to order by in the query but in master i get it.
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-793069330
@ocket8888 - looks like these query params are no longer respected:
```
GET api/4.0/deliveryservice_requests?orderby=createdAt&sortOrder=desc
```
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-790951196
> For some reason I am unable to change the status. Example:
Looks like a bad rebase from the old PR. Should be working now.
----------------------------------------------------------------
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 edited a comment on pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-793069330
@ocket8888 - looks like these query params are no longer respected:
```
GET /deliveryservice_requests?orderby=createdAt&sortOrder=desc
```
----------------------------------------------------------------
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 removed a comment on pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 removed a comment on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-796274391
> I was able to get `orderby` and `sortOrder` to work. I even tried specifically ordering by `createdAt`. Worked fine.
yeah i can't get the `sortOrder` to stick when i do GET /api/4.0/deliveryservice_requests?orderby=createdAt&sortOrder=desc&limit=10 in your branch, i get this query:
```
SELECT
a.username AS author,
e.username AS lastEditedBy,
s.username AS assignee,
r.assignee_id,
r.author_id,
r.change_type,
r.created_at,
r.id,
r.last_edited_by_id,
r.last_updated,
r.deliveryservice,
r.status
FROM deliveryservice_request r
JOIN tm_user a ON r.author_id = a.id
LEFT OUTER JOIN tm_user s ON r.assignee_id = s.id
LEFT OUTER JOIN tm_user e ON r.last_edited_by_id = e.id
WHERE (
CAST(r.deliveryservice->>'tenantId' AS BIGINT) = ANY(CAST(:accessibleTenants AS BIGINT[]))
)
ORDER BY r.created_at
LIMIT 10
```
^^ see? no DESC attached to order by in the query but in master i get it.
----------------------------------------------------------------
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 #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#discussion_r587748439
##########
File path: docs/source/glossary.rst
##########
@@ -115,7 +115,9 @@ Glossary
.. seealso:: See :ref:`delivery-services` for a more in-depth explanation of :dfn:`Delivery Services`.
Delivery Service Request
+ Delivery Service REquests
Review comment:
typo
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-790914251
For some reason I am unable to change the status. Example:
```
Request URL: https://localhost:8442/api/4.0/deliveryservice_requests/4641/status
Request Method: PUT
Request Payload: {status: "submitted"}
Status Code: 200 OK
```
```
Request URL: https://localhost:8442/api/4.0/deliveryservice_requests/4641/status
Request Method: GET
Status Code: 200 OK
{"response":"draft"}
```
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-772119111
Yeah, I kinda expected that. Full disclosure: I didn't run the tests. Wanted the CI actions to do it for me.
I'm on it.
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-794237350
This doesnt seem right. here's the change log message for updating a DSR:
```
Updated Delivery Service Request of type update for Delivery Service ''
```
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-772119111
Yeah, I kinda expected that. Full disclosure: I didn't run the tests. Wanted the CI actions to do it for me.
I'm on it.
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-795979882
I was able to get `orderby` and `sortOrder` to work. I even tried specifically ordering by `createdAt`. Worked fine.
----------------------------------------------------------------
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 pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-796289704
Actually it looks like the ordering was only working for fields other than `createdAt`, and when I tested that one it was only ordered by pure coincidence. Should be fixed now. Also added documentation for the sorting and pagination query parameters.
----------------------------------------------------------------
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 edited a comment on pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489#issuecomment-794237350
This doesnt seem right. here's the change log message for updating a DSR:
```
Updated Delivery Service Request of type update for Delivery Service ''
```
Same with deleting a DSR:
```
Deleted Delivery Service Request of type update for Delivery Service ''
```
----------------------------------------------------------------
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 merged pull request #5489: Non-"CRUD-er" DSRs
Posted by GitBox <gi...@apache.org>.
mitchell852 merged pull request #5489:
URL: https://github.com/apache/trafficcontrol/pull/5489
----------------------------------------------------------------
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