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/18 19:31:12 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5539: Remove api v1
ocket8888 opened a new pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539
## What does this PR (Pull Request) do?
- [x] This PR is not related to any Issue
This PR removes API version 1 entirely, including documentation and tests.
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Ops Client (Go)
- Traffic Ops
- CI tests
## What is the best way to verify this PR?
verify existing API tests pass for all versions greater than 1, make a request to APIv1 and verify it doesn't succeed.
## The following criteria are ALL met by this PR
- [x] Tests are unnecessary
- [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] zrhoffman edited a comment on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-811280726
The tests all pass now<strike>, but we should not merge it until #5377 is done</strike>.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-811280726
The tests all pass now, but we should not merge it until #5377 is done.
--
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 edited a comment on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876733076
> Usages in cache-config/ are in GoDoc comments
> Usage in lib/go-tc is in GoDoc comments
> In tosession.py it's in a comment on an example - which itself works afaik - and doesn't appear in any documentation.
> Usage in traffic_ops/testing/ is in non-documenting comments
> The URL used in the DS Stats test is arbitrary
Updating all of these makes sense to me, IMO.
> cfg_test.pl is a script that, I believe, has not worked for longer than your involvement in the project. Possibly longer than mine. It was replaced by the compare tool, which has since been removed because it no longer serves any purpose. This script should not exist.
Sounds like it could have been removed in the Perlectomy. Removing it with the rest of API v1 is the next best thing, right?
---
> Usage in Grove is in a comment that wouldn't even appear in package documentation. I can get rid of it if you want, but it's not affecting anything.
https://github.com/apache/trafficcontrol/blob/1c72915665192d9ffa4d08e5ba202e044b38c2be/grove/grovetccfg/grovetccfg.go#L195
I'm seeing more API v1 stuff lower down:
https://github.com/apache/trafficcontrol/blob/1c72915665192d9ffa4d08e5ba202e044b38c2be/grove/grovetccfg/grovetccfg.go#L232-L265
Removing the Grove API v1 stuff would make more sense in a separate PR.
> Usage in the ORT.py package is irrelevant, since that is due for removal.
> I don't tend to touch anything in infrastructure/docker (except in the build subdirectory) because those Dockerfiles are archaic, they're parsing that API output in a way that may not be trivial (e.g. getting/setting server IP addresses), and their use is nowhere recommended or even documented.
> Usage in load-test.jsx is not referring to a Traffic Ops host - you can tell because it's passing a hostname for a Traffic Ops instance as a query parameter. test/router/server.go is the server serving that API, so the only real change necessary should be in there. But on the other hand - that test suite doesn't work. All browsers I've tried (Firefox, Chrome, Opera, all at latest versions) will refuse to load the .jsx file on the grounds of CORS request failure. If those tests are intended to work, they'll need to be reworked anyway, so the changes could just take place then.
> Swagger is not actually supported by Traffic Ops. The traffic_ops/traffic_ops_golang/swaggerdocs/ directory and any and all sub-packages thereof should be deleted, in my opinion. Unless someone is going to make them work, in which case they ought to update them to be correct themselves.
Agreed that taking care of these outside of #5539 makes sense.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876733076
> Usages in cache-config/ are in GoDoc comments
> Usage in lib/go-tc is in GoDoc comments
> In tosession.py it's in a comment on an example - which itself works afaik - and doesn't appear in any documentation.
> Usage in traffic_ops/testing/ is in non-documenting comments
> The URL used in the DS Stats test is arbitrary
Updating all of these makes sense to me, IMO.
> cfg_test.pl is a script that, I believe, has not worked for longer than your involvement in the project. Possibly longer than mine. It was replaced by the compare tool, which has since been removed because it no longer serves any purpose. This script should not exist.
Sounds like it could have been removed in the Perlectomy. Removing it with the rest of API v1 is the next best thing, right?
---
> Usage in Grove is in a comment that wouldn't even appear in package documentation. I can get rid of it if you want, but it's not affecting anything.
https://github.com/apache/trafficcontrol/blob/1c72915665192d9ffa4d08e5ba202e044b38c2be/grove/grovetccfg/grovetccfg.go#L195
I'm seeing more API v1 stuff lower down:
https://github.com/apache/trafficcontrol/blob/1c72915665192d9ffa4d08e5ba202e044b38c2be/grove/grovetccfg/grovetccfg.go#L232-L265
Removing the Grove API v1 stuff would make more sense in a separate PR.
> Usage in the ORT.py package is irrelevant, since that is due for removal.
> I don't tend to touch anything in infrastructure/docker (except in the build subdirectory) because those Dockerfiles are archaic, they're parsing that API output in a way that may not be trivial (e.g. getting/setting server IP addresses), and their use is nowhere recommended or even documented.
> Usage in load-test.jsx is not referring to a Traffic Ops host - you can tell because it's passing a hostname for a Traffic Ops instance as a query parameter. test/router/server.go is the server serving that API, so the only real change necessary should be in there. But on the other hand - that test suite doesn't work. All browsers I've tried (Firefox, Chrome, Opera, all at latest versions) will refuse to load the .jsx file on the grounds of CORS request failure. If those tests are intended to work, they'll need to be reworked anyway, so the changes could just take place then.
> Swagger is not actually supported by Traffic Ops. The traffic_ops/traffic_ops_golang/swaggerdocs/ directory and any and all sub-packages thereof should be deleted, in my opinion. Unless someone is going to make them work, in which case they ought to update them to be correct themselves.
Agreed that taking care of these outside of #5339 makes sense.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#discussion_r669958207
##########
File path: traffic_control/clients/README.md
##########
@@ -8,8 +8,11 @@ There are two client libraries supported:
* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_control/clients/python/trafficops)
## Golang
-### TO API v1.5 _(Deprecated)_
-* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/v1-client)
+### TO API v2 _(Deprecated)_
+* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/client)
+
+### TO API v3 _(Deprecated)_
Review comment:
My understanding was that versions 2 and 3 are deprecated once 4 is released Then with ATCv7 we can remove them and deprecate 4 assuming 5 was under development after ATCv6's release and released with ATCv7 as the new stable API.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876751929
Yes. `RestApiSession` is a class that abstracts making request to an arbitrary REST API, it is not specific to Traffic Ops in any way. TO-specific code is in `tosession.py`.
```python3
class RestApiSession(object):
"""
This class represents a login session with a generic REST API server. It provides base
functionality inherited by :class:`TOSession`.
"""
...
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#discussion_r667094188
##########
File path: traffic_control/clients/README.md
##########
@@ -8,8 +8,11 @@ There are two client libraries supported:
* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_control/clients/python/trafficops)
## Golang
-### TO API v1.5 _(Deprecated)_
-* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/v1-client)
+### TO API v2 _(Deprecated)_
+* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/client)
+
+### TO API v3 _(Deprecated)_
+* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/client)
Review comment:
Should be `v3-client`
##########
File path: traffic_control/clients/README.md
##########
@@ -8,8 +8,11 @@ There are two client libraries supported:
* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_control/clients/python/trafficops)
## Golang
-### TO API v1.5 _(Deprecated)_
-* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/v1-client)
+### TO API v2 _(Deprecated)_
+* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/client)
Review comment:
Should be `v2-client`
##########
File path: traffic_control/clients/README.md
##########
@@ -8,8 +8,11 @@ There are two client libraries supported:
* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_control/clients/python/trafficops)
## Golang
-### TO API v1.5 _(Deprecated)_
-* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/v1-client)
+### TO API v2 _(Deprecated)_
+* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/client)
+
+### TO API v3 _(Deprecated)_
Review comment:
TO API v3 is deprecated? I thought it's the "stable" version until API v2 is removed and API v5 is added.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876780579
> [I]f we're going to change it, it should be to a version that isn't a valid TO API version (getting rid of it wouldn't be good, it's including an example that shows an API version in the base path segment in addition to an example that doesn't so that the reader understands that they may or may not include a version in the base path at their discretion) like, say, `/api/v1.2`?
IMO it still looks similar to a TO API version. `/api/latest`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#discussion_r669964628
##########
File path: traffic_control/clients/README.md
##########
@@ -8,8 +8,11 @@ There are two client libraries supported:
* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_control/clients/python/trafficops)
## Golang
-### TO API v1.5 _(Deprecated)_
-* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/v1-client)
+### TO API v2 _(Deprecated)_
+* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/client)
+
+### TO API v3 _(Deprecated)_
Review comment:
That makes sense, but in order to remove them API v2 and API v3 in ATC 7, a separate PR will need to mark all of the endpoints as deprecated in the documentation for ATC 6.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876768912
> Changing `1.2` to a version that we have never removed from Traffic Ops still seems like an improvement, even if that means changing it to a version Traffic Ops will never use.
If we do change it, it should specifically be to a version Traffic Ops would never use. I don't want this thing that has nothing to do with Traffic Ops to be a part of the upkeep of eliminating a Traffic Ops API version that happens to have path segments that look similar to it. So if we're going to change it, it should be to a version that isn't a valid TO API version (getting rid of it wouldn't be good, it's including an example that shows an API version in the base path segment in addition to an example that doesn't so that the reader understands that they may or may not include a version in the base path at their discretion) like, say, `/api/v1.2`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876749919
> The parameter documentation for `api_base_path` in `restapi.py` doesn't need to be updated. That is not referring to the Traffic Ops API.
You are referring to
https://github.com/apache/trafficcontrol/blob/b64fcd5bf675c73ea1d085ee40245fcbd92e0df5/traffic_control/clients/python/trafficops/restapi.py#L141-L148
?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#discussion_r669958754
##########
File path: traffic_portal/app/src/scripts/config.js
##########
@@ -23,4 +23,4 @@
angular.module('config', [])
-.constant('ENV', { api: { root:'/api/4.0/', legacy: '/api/1.5/', stable: "/api/3.1/" } });
Review comment:
It could, but it wouldn't matter because that's unused. It was added to allow TP to still show cache server configuration files after moving to APIv2 - since those routes no longer work we have no further use for "legacy" afaik.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876698593
Usages in `cache-config/` are in GoDoc comments - it looks safe to change them since the information they're conveying isn't actually specific to that API version. I can do that if you want, but it shouldn't affect anything.
Usage in docs example has been updated
Usage in the ORT.py package is irrelevant, since that is due for removal. Pointless to change now, since it can't possibly work now that `atstccfg` has been removed, and whatever I do will be deleted anyway.
Usage in Grove is in a comment that wouldn't even appear in package documentation. I can get rid of it if you want, but it's not affecting anything.
I don't tend to touch anything in `infrastructure/docker` (except in the `build` subdirectory) because those Dockerfiles are archaic, they're parsing that API output in a way that may not be trivial (e.g. getting/setting server IP addresses), and their use is nowhere recommended or even documented. I don't even know if those work. I can change it, but it might break things in weird and unexpected ways, which doesn't seem worth it for something we don't even really support.
Usage in `lib/go-tc` is in GoDoc comments, which I can change if you want.
Usage in `load-test.jsx` is not referring to a Traffic Ops host - you can tell because it's passing a hostname for a Traffic Ops instance as a query parameter. `test/router/server.go` is the server serving that API, so the only real change necessary should be in there. But on the other hand - that test suite doesn't work. All browsers I've tried (Firefox, Chrome, Opera, all at latest versions) will refuse to load the `.jsx` file on the grounds of CORS request failure. If those tests are intended to work, they'll need to be reworked anyway, so the changes could just take place then.
`cfg_test.pl` is a script that, I believe, has not worked for longer than your involvement in the project. Possibly longer than mine. It was replaced by the `compare` tool, which has since been removed because it no longer serves any purpose. This script should not exist.
Usage in restapi.py is for a generic API, an API base URL containing an `api/1` segment is perfectly valid in that context. In `tosession.py` it's in a comment on an example - which itself works afaik - and doesn't appear in any documentation. I can change it if you want, but it's not affecting anything.
Usage in `copy-pgdata.sh` has been updated. I thought that was for migrating from a mysql database to postgresql, but the comment header implies otherwise. Regardless, the usages are trivial enough to change easily.
Usage in `traffic_ops/testing/` is in non-documenting comments, which I can change if you want but it's not affecting anything.
Usage in `traffic_ops/traffic_ops_golang/` in GoDoc comments has been updated, as it was incorrect - except for one instance, which is still technically correct. One usage as a hard-coded value in a response header and one usage in a test have both been updated. The usages in the routing test are specifically checking for APIv1 not existing, and so should remain. The URL used in the DS Stats test is arbitrary, I can change it, but it won't be any more or less valid; only the headers on the request actually matter.
Swagger is not actually supported by Traffic Ops. The `traffic_ops/traffic_ops_golang/swaggerdocs/` directory and any and all sub-packages thereof should be deleted, in my opinion. Unless someone is going to make them work, in which case they ought to update them to be correct themselves. But I don't think that's actually possible, in general, without significant code changes and some way to integrate the output with our existing documentation system (which doesn't exist afaik).
Usage in TP was previously removed, but must have been re-introduced during rebase with the commit that added/updated the `stable` API version.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876761507
> The parameter documentation for `api_base_path` in `restapi.py` doesn't need to be updated. That is not referring to the Traffic Ops API.
Changing `1.2` to a version that we have never removed from Traffic Ops still seems like an improvement, even if that means changing it to a version Traffic Ops will never use.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876802580
I can't change those without making breaking changes to structures that are still in use after the removal of APIv1
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-811284745
> The tests all pass now, but we should not merge it until #5377 is done.
I made this not a draft because someone had told me that was done... I'll investigate to see if #5377 can be closed
--
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 #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-781772919
This can't be done until #5541 is closed
----------------------------------------------------------------
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 #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876747370
The parameter documentation for `api_base_path` in `restapi.py` doesn't need to be updated. That is not referring to the Traffic Ops API.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876819443
The [`traffic_router/core/src/test/resources/api/1.3`](https://github.com/apache/trafficcontrol/tree/f07cf9211a/traffic_router/core/src/test/resources/api/1.3) directory can be removed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876740022
Oops, missed a response.
> Usage in restapi.py is for a generic API, an API base URL containing an `api/1` segment is perfectly valid in that context. In `tosession.py` it's in a comment on an example - which itself works afaik - and doesn't appear in any documentation. I can change it if you want, but it's not affecting anything.
https://github.com/apache/trafficcontrol/blob/b64fcd5bf675c73ea1d085ee40245fcbd92e0df5/traffic_control/clients/python/trafficops/restapi.py#L141
https://github.com/apache/trafficcontrol/blob/b64fcd5bf675c73ea1d085ee40245fcbd92e0df5/traffic_control/clients/python/trafficops/restapi.py#L147
Updating these makes sense to me, 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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876761507
> The parameter documentation for `api_base_path` in `restapi.py` doesn't need to be updated. That is not referring to the Traffic Ops API.
Changing `1.2` to a version that Traffic Ops has never used still seems like an improvement, even if that means changing it to a version Traffic Ops will never use.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#discussion_r669925279
##########
File path: traffic_portal/app/src/scripts/config.js
##########
@@ -23,4 +23,4 @@
angular.module('config', [])
-.constant('ENV', { api: { root:'/api/4.0/', legacy: '/api/1.5/', stable: "/api/3.1/" } });
Review comment:
Why remove the `legacy` property? Can it not be incremented to API v2?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876758607
> I'm seeing more API v1 stuff lower down:
I don't know what I can do about that. The comment says to remove that block when the API is updated to 1.3, but we're moving past that here. Plus from what I can tell the information it's gathering is actually necessary for the utility to do its work, so idk how I can remove it without breaking everything. It seems like Grove may need to be refactored, only @rob05c could possibly know for sure.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman merged pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#discussion_r666537279
##########
File path: traffic_ops/traffic_ops_golang/trafficstats/util_test.go
##########
@@ -53,7 +53,7 @@ func TestTSConfigFromRequest(t *testing.T) {
},
}
- r, e := http.NewRequest(http.MethodGet, "https://example.test/api/1.4/deliveryservice_stats", nil)
+ r, e := http.NewRequest(http.MethodGet, "https://example.test/api/whatever version we're on now/deliveryservice_stats", nil)
Review comment:
excellent
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#discussion_r669920621
##########
File path: traffic_control/clients/README.md
##########
@@ -8,8 +8,11 @@ There are two client libraries supported:
* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_control/clients/python/trafficops)
## Golang
-### TO API v1.5 _(Deprecated)_
-* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/v1-client)
+### TO API v2 _(Deprecated)_
+* [Documentation](https://github.com/apache/trafficcontrol/tree/master/traffic_ops/client)
+
+### TO API v3 _(Deprecated)_
Review comment:
> TO API v3 is deprecated? I thought it's the "stable" version until API v2 is removed and API v5 is added.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 edited a comment on pull request #5539: Remove api v1
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5539:
URL: https://github.com/apache/trafficcontrol/pull/5539#issuecomment-876751929
Yes. `RestApiSession` is a class that abstracts making requests to an arbitrary REST API, it is not specific to Traffic Ops in any way. TO-specific code is in `tosession.py`.
```python3
class RestApiSession(object):
"""
This class represents a login session with a generic REST API server. It provides base
functionality inherited by :class:`TOSession`.
"""
...
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org