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/03/25 20:21:21 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5678: Delivery Service Request "originals"
ocket8888 opened a new pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678
## What does this PR (Pull Request) do?
- [x] This PR is not related to any Issue.
TThis PR overhauls how DSRs work (in APIv4). Instead of a single `deliveryservice` property, DSRs will now have `requested` and `original`.
`requested` is the Delivery Service as it is requested to exist. For `delete`-type DSRs, this property will not exist in responses and will be ignored in requests. In most senses, it's pretty analogous to the old `deliveryservice` property.
`original` is the Delivery Service as it exists/existed before the DSR is/was/would have been applied. This property does not exist on `create`-type DSRs. For non-`create` DSRs that are "open", this is either given in the request (for `delete` DSRs) or generated by Traffic Ops for responses from the current DS that matches the DSR's `requested`'s XMLID (for `update` DSRs). When a DSR is "closed", the original is saved from the current DS so that even if that DS changes in the future, the actual change enacted by the now-closed DSR is preserved.
Also, a couple more changes: Assignee ID, Author ID, and Last Edited By ID are no longer present on objects returned by the API.
For a (hopefully) complete explanation of the new model and interface, refer to the PR's docs changes.
Finally, this makes TP use the stable API 3.1 endpoints for DSR-related operations, to avoid shoving TP work into this PR.
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Ops
- Traffic Portal
## What is the best way to verify this PR?
Make sure the tests pass, make sure working with DSRs in Traffic Portal is unaffected.
## 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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-810505737
> Oh yeah, you can't do that I forgot
it's fine. you can probably leave it the way it is
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-808452150
getting error on migration @ocket8888
```
goose: migrating db environment 'development', current version: 2021031400000000, target: 2021032600000000
2021/03/26 13:06:34 FAIL 2021032600000000_dsrs_originals.sql (pq: check constraint "appropriate_requested_and_original_for_change_type" is violated by some row), quitting migration.
Can't run goose: exit status 1
```
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-810465745
> For update requests that change the XMLID, to which XMLID does the "top-level" XMLID property refer?
well, you can't ever change xmlId so it really feels like a top-level property of a DSR imo...but i guess you could on a create....
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#discussion_r604185390
##########
File path: traffic_portal/app/src/scripts/config.js
##########
@@ -23,6 +23,4 @@
angular.module('config', [])
-.constant('ENV', { api: { root:'/api/4.0/', legacy: '/api/1.5/' } })
-
-;
+.constant('ENV', { api: { root:'/api/4.0/', legacy: '/api/1.5/', stable: "/api/3.1" } });
Review comment:
stable needs a trailing /
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-810487596
Oh yeah, you can't do that I forgot
--
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 edited a comment on pull request #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-818070987
> looking pretty good so far but i hit this potential bug:
>
> i fulfilled a 'delete' request which
>
> 1. set the DSR to pending and
>
> 2. deletes the DS
>
>
> but then i tried to set the DSR to 'complete' and got this:
>
> ```
> ERROR: api.go:240: 2021-04-09T16:32:32.924133Z: 127.0.0.1:56897 expected exactly one DS with XMLID 'dns-345346546', found: 0
> ```
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] ocket8888 commented on pull request #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-810453243
> for changetype=delete, requested is not applicable so the requested property is omitted from the json response, correct? can you not omit it and just set its value to null instead. it's weird imo to have a different json structure for different types.
> for changetype=create, original is not applicable so the original property is omitted from the json response, correct? can you not omit it and just set its value to null instead. it's weird imo to have a different json structure for different types.
The way our API is implemented, those are totally equivalent. You can pass `null` or not pass it at all. Both have the same result.
> for a DSR, the xmlId is an interesting piece of information but i have to look in either the requested or original value (depending on changetype) to find it. should we just promote it to a first level property of all DSRs?
The problem with that is that the original and requested already have XMLIDs, so at best we're specifying the same information twice. But what do you do if they're not the same? Reject the request? Or treat one as the truth? If you don't treat the original/requested as the truth, then your DSRs are lying to you. So if the XMLID must already exist and be the correct, canonical XMLID on the requested/original, why add it somewhere else, too? For update requests that change the XMLID, to which XMLID does the "top-level" XMLID property refer?
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-812685640
> This no longer works:
> `GET /deliveryservice_requests?xmlId=foo&status=submitted`
> It was a pretty handy way to see if a ds had "open" dsrs... and TP leverages that query param to "auto-complete" DSRs.
Which part of that no longer works? I can filter by both xmlId and status. I tested this just now by:
1. created an "update" DSR for `demo1` in "draft" status
2. requested `deliveryservice_requests?xmlId=demo1` - returns that DSR
3. requested `deliveryservice_requests?xmlId=demo2` - returns an empty array
4. requested `deliveryservice_requests?xmlId=demo1&status=submitted` - returns an empty array
5. update the status to "submitted" (which succeeds but returns a 500, for some reason I still have to look into)
6. requested `deliveryservice_requests?xmlId=demo1` - returns that DSR
7. requested `deliveryservice_requests?xmlId=demo2` - returns an empty array
8. requested `deliveryservice_requests?xmlId=demo1&status=submitted` - returns that DSR
So it's all working fine from what I can see. Plus I'm pretty confident that the API tests exercise at least the XMLID param.
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-810465745
> For update requests that change the XMLID, to which XMLID does the "top-level" XMLID property refer?
well, you can't ever change xmlId so it really feels like a top-level property of a DSR imo
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-818070987
> looking pretty good so far but i hit this potential bug:
>
> i fulfilled a 'delete' request which
>
> 1. set the DSR to pending and
>
> 2. deletes the DS
>
>
> but then i tried to set the DSR to 'complete' and got this:
>
> ```
> ERROR: api.go:240: 2021-04-09T16:32:32.924133Z: 127.0.0.1:56897 expected exactly one DS with XMLID 'dns-345346546', found: 0
> ```
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-810465745
> For update requests that change the XMLID, to which XMLID does the "top-level" XMLID property refer?
well, you can't ever change xmlId
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-810505737
> Oh yeah, you can't do that I forgot
it's fine. you can probably leave it the way it is with regard to top-level xmlId
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-812713149
Yeah, updating "delete" DSRs is working now, too.
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
mitchell852 merged pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678
--
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 #5678: Delivery Service Request "originals"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5678:
URL: https://github.com/apache/trafficcontrol/pull/5678#issuecomment-812711926
> updating an update DSR - fails
I believe I have fixed this, testing updating a "delete" DSR 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