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

Add asn field to server#7023

Merged
ocket8888 merged 8 commits intoapache:masterfrom
srijeet0406:add_asn_to_server
Sep 7, 2022
Merged

Add asn field to server#7023
ocket8888 merged 8 commits intoapache:masterfrom
srijeet0406:add_asn_to_server

Conversation

@srijeet0406
Copy link
Copy Markdown
Contributor

@srijeet0406 srijeet0406 commented Aug 16, 2022

This PR is not related to any issue. It adds the ASN field to the server struct, so that users can use asn as a query param to GET servers.

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops
  • Traffic Portal

What is the best way to verify this PR?

Run TO and TP locally.
Hit the 4.1 GET /servers route and make sure that the asns field shows up in the response.
Hit the same endpoint and add asn=<> as a query param.
Make sure you see the correct servers in the response.
Make sure the other routes DO NOT return this new field.
Make sure all other CRUD routes for servers work fine.
Test TP to verify that you can show/ hide the new field in servers.

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

  • master

PR submission checklist

@srijeet0406 srijeet0406 marked this pull request as draft August 16, 2022 16:08
@srijeet0406 srijeet0406 self-assigned this Aug 16, 2022
@srijeet0406 srijeet0406 added Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1 medium impact impacts a significant portion of a CDN, or has the potential to do so labels Aug 16, 2022
@srijeet0406 srijeet0406 marked this pull request as ready for review August 16, 2022 16:37
ocket8888
ocket8888 suggested changes Aug 18, 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.

We have a build failure/warning in the docs on master (it's recoverable which is why the action reports a success), so I'm tryna put some extra effort into checking out the docs of things that I otherwise don't look at to prevent stuff like that from slipping through again. I don't intend to review the rest of the code.

Comment thread docs/source/api/v4/servers.rst Outdated
Comment thread lib/go-tc/servers.go Outdated
rimashah25
rimashah25 suggested changes Aug 22, 2022
View reviewed changes
Comment thread lib/go-tc/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/routing/routes.go Outdated
Comment thread lib/go-tc/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers_test.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers_test.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers_test.go Outdated
Comment thread traffic_ops/testing/api/v4/tc-fixtures.json Outdated
rimashah25
rimashah25 reviewed Aug 22, 2022
View reviewed changes
Comment thread traffic_ops/testing/api/v4/servers_test.go Outdated
ericholguin
ericholguin reviewed Aug 22, 2022
View reviewed changes
Comment thread traffic_ops/testing/api/v4/servers_test.go Outdated
rimashah25
rimashah25 reviewed Aug 23, 2022
View reviewed changes
Comment thread traffic_portal/app/src/common/modules/table/servers/TableServersController.js Outdated
Copy link
Copy Markdown
Contributor

@rimashah25 rimashah25 Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: One other thing in TP, under the column it shows correct spacing but when hovered over it, spacing between , and number disappears
image

rimashah25
rimashah25 approved these changes Aug 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rimashah25 rimashah25 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM. Verified that one can see asn field only in 4.1 /servers API (GET) route and all other REST API calls for /servers work fine. asn used as query param in 4.1 GET servers also gives the correct response.
The asn column is now seen in TP as a list if a cachegroup has more than one asn assigned to it.
Unit tests and Integration tests (v3/v4) pass successfully on local.

ocket8888
ocket8888 suggested changes Aug 24, 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.

The ServerV4 type alias in lib/go-tc/servers.go still says that the latest version of a server is ServerV40. This means that other API endpoints that show servers - e.g. /deliveryservices/{{ID}}/servers/eligible etc. - all now have different representations of a server than /servers. This is, in my opinion, bad. An API object should have exactly one representation.

Comment thread docs/source/api/v4/servers.rst Outdated
Comment thread docs/source/api/v4/servers.rst Outdated
Comment thread docs/source/api/v4/servers.rst Outdated
@srijeet0406
Copy link
Copy Markdown
Contributor Author

srijeet0406 commented Aug 24, 2022

The ServerV4 type alias in lib/go-tc/servers.go still says that the latest version of a server is ServerV40. This means that other API endpoints that show servers - e.g. /deliveryservices/{{ID}}/servers/eligible etc. - all now have different representations of a server than /servers. This is, in my opinion, bad. An API object should have exactly one representation.

I've updated the ServerV4 to point to ServerV41 now. However, if you hit the other endpoints, for example, /deliveryservices/{{ID}}/servers/eligible, you'd still get a server representation without the ASN field. That field is only added for the GET routes of /servers. Because there is not 4.1 equivalent route for the other endpoints (for example, /deliveryservices/{{ID}}/servers/eligible), they still return the version 4.0 representation of the server object.

@srijeet0406
Copy link
Copy Markdown
Contributor Author

srijeet0406 commented Aug 25, 2022

IDK why the go fmt GHA keeps failing. It's checking a piece of code that's not even a part of this PR.

@ocket8888
Copy link
Copy Markdown
Contributor

ocket8888 commented Aug 25, 2022

IDK why the go fmt GHA keeps failing. It's checking a piece of code that's not even a part of this PR.

That's because #6960 was erroneously merged with a go fmt failure.

@ocket8888
Copy link
Copy Markdown
Contributor

ocket8888 commented Aug 25, 2022

A fix is now open #7038

@srijeet0406
Copy link
Copy Markdown
Contributor Author

srijeet0406 commented Aug 25, 2022

The ServerV4 type alias in lib/go-tc/servers.go still says that the latest version of a server is ServerV40. This means that other API endpoints that show servers - e.g. /deliveryservices/{{ID}}/servers/eligible etc. - all now have different representations of a server than /servers. This is, in my opinion, bad. An API object should have exactly one representation.

This will be addressed in a follow up PR.

ocket8888
ocket8888 approved these changes Sep 7, 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 look good now, and all actions pass

@ocket8888 ocket8888 merged commit 5e8255f into apache:master Sep 7, 2022
@srijeet0406 srijeet0406 mentioned this pull request Sep 7, 2022
Closed
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
@srijeet0406
Add asn field to server (apache#7023)
1b21271 
* Adding ASN to Server struct

* doc changes

* fix typo

* add godoc

* address code review comments

* address code review changes

* address code review comments

* remove extra doc line
@srijeet0406 srijeet0406 mentioned this pull request Oct 11, 2022
Merged
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@ericholguin ericholguin ericholguin left review comments

@ocket8888 ocket8888 ocket8888 approved these changes

@rimashah25 rimashah25 rimashah25 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@srijeet0406 srijeet0406

Labels

medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@srijeet0406 @ocket8888 @ericholguin @rimashah25