Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Adding the asn field to all GET endpoints that return a server object#7073

Merged
ocket8888 merged 6 commits intoapache:masterfrom
srijeet0406:add-asn-to-all-server-endpoints
Sep 29, 2022
Merged

Adding the asn field to all GET endpoints that return a server object#7073
ocket8888 merged 6 commits intoapache:masterfrom
srijeet0406:add-asn-to-all-server-endpoints

Conversation

@srijeet0406
Copy link
Copy Markdown
Contributor

@srijeet0406 srijeet0406 commented Sep 19, 2022 •
edited
Loading

This PR closes #7064 and #7085

Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

Run Traffic Ops locally.
Make requests for any api version other than 4.1 to any endpoint that returns a server structure, for example:
GET deliveryservices/{id}/servers/eligible
GET deliveryservices/{id}/servers

Make sure you do not see the asns field in the response.

Run the same test for api version 4.1 and make sure that you see the asns field in the response this time.

Verify that all unit and API tests pass.

If this is a bugfix, which Traffic Control versions contained the bug?

  • master

PR submission checklist

@srijeet0406 srijeet0406 added bug something isn't working as intended Traffic Ops related to Traffic Ops medium impact impacts a significant portion of a CDN, or has the potential to do so low difficulty the estimated level of effort to resolve this issue is low labels Sep 19, 2022
@srijeet0406 srijeet0406 self-assigned this Sep 19, 2022
@srijeet0406 srijeet0406 removed the low difficulty the estimated level of effort to resolve this issue is low label Sep 19, 2022
ocket8888
ocket8888 suggested changes Sep 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs docs updates

Comment thread traffic_ops/traffic_ops_golang/deliveryservice/eligible.go
@ocket8888 ocket8888 assigned ocket8888 and unassigned srijeet0406 Sep 19, 2022
zrhoffman
zrhoffman suggested changes Sep 19, 2022
View reviewed changes
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/eligible.go
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/eligible_test.go Outdated
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/servers/servers_test.go Outdated
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/servers/servers_test.go Outdated
Comment thread docs/source/api/v4/deliveryservices_id_servers.rst Outdated
Comment thread docs/source/api/v4/deliveryservices_id_servers_eligible.rst Outdated
Comment thread docs/source/api/v5/deliveryservices_id_servers.rst Outdated
zrhoffman
zrhoffman approved these changes Sep 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'll wait for @ocket8888 to approve before merging (unless he merges it first)

@zrhoffman zrhoffman added improvement The functionality exists but it could be improved in some way. and removed bug something isn't working as intended labels Sep 21, 2022
ocket8888
ocket8888 suggested changes Sep 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs changes all look good (comments are nitpicks), but changes still need to be made to the /servers/{{ID}} endpoint documentation

Comment thread lib/go-tc/deliveryservice_servers.go Outdated
Comment thread docs/source/api/v5/deliveryservices_id_servers.rst Outdated
Comment thread docs/source/api/v5/deliveryservices_id_servers_eligible.rst Outdated
@srijeet0406
Copy link
Copy Markdown
Contributor Author

srijeet0406 commented Sep 21, 2022

Docs changes all look good (comments are nitpicks), but changes still need to be made to the /servers/{{ID}} endpoint documentation

While fixing the docs of that endpoint, I noticed that those endpoints returning the server structures were not consistent with the api versions they were being called from. So I've fixed that in this PR as well.

rimashah25
rimashah25 suggested changes Sep 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rimashah25 rimashah25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes to get specific version of server object with different version of API call looks good. Tested the changes on localhost with requested selectQuery change and the output was as expected.

Comment thread traffic_ops/traffic_ops_golang/server/servers.go
ocket8888
ocket8888 reviewed Sep 23, 2022
View reviewed changes
Comment thread lib/go-tc/deliveryservice_servers.go Outdated
rimashah25
rimashah25 approved these changes Sep 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rimashah25 rimashah25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the changes for correct server struct response for different version of API calls.

@ocket8888 ocket8888 merged commit 463d595 into apache:master Sep 29, 2022
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
@srijeet0406
Adding the asn field to all GET endpoints that return a server …
d48630e 
…object (apache#7073)

* Adding the  field to all  endpoints

* Adding doc changes

* address code review comments

* fix testreadservers

* fix server representations to be version specific

* code review changes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@ocket8888 ocket8888 ocket8888 approved these changes

@zrhoffman zrhoffman zrhoffman approved these changes

@rimashah25 rimashah25 rimashah25 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ocket8888 ocket8888

Labels

improvement The functionality exists but it could be improved in some way. medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Ops related to Traffic Ops

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

All endpoints need to account for the ASN field in servers

4 participants

@srijeet0406 @ocket8888 @zrhoffman @rimashah25