Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add asn field to server #7023

Merged
merged 8 commits into from
Sep 7, 2022
Merged

Add asn field to server #7023

merged 8 commits into from
Sep 7, 2022

Conversation

srijeet0406
Copy link
Contributor

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 requested changes Aug 18, 2022
View reviewed changes
Copy link
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.

rimashah25
rimashah25 requested changes Aug 22, 2022
View reviewed changes
rimashah25
rimashah25 reviewed Aug 23, 2022
View reviewed changes
traffic_portal/app/src/common/modules/table/servers/TableServersController.js
@@ -155,6 +155,11 @@ var TableServersController = function(tableName, servers, filter, $scope, $state
field: "offlineReason",
hide: true
},
{
Copy link
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
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 requested changes Aug 24, 2022
View reviewed changes
Copy link
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.

@srijeet0406
Copy link
Contributor Author

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
Contributor Author

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
Contributor

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
Contributor

A fix is now open #7038

@srijeet0406
Copy link
Contributor Author

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
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 join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericholguin ericholguin ericholguin left review comments

@ocket8888 ocket8888 ocket8888 approved these changes

@rimashah25 rimashah25 rimashah25 approved these changes

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