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 2020/11/05 17:07:36 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5248: Fix servers client methods changing call signatures
ocket8888 opened a new pull request #5248:
URL: https://github.com/apache/trafficcontrol/pull/5248
## What does this PR (Pull Request) do?
- [x] This PR is not related to any Issue
At some point (probably when I added interfaces to servers) a bunch of TO client methods dealing with servers changed their call signatures (I think it was always from `tc.Server` to `tc.ServerNullable`. This not only fixes that, but also changes client server methods that use APIv3 structures to now use `tc.ServerV30` instead of the deprecated alias `tc.ServerNullable`.
## Which Traffic Control components are affected by this PR?
- Traffic Ops Client (Go)
- Traffic Monitor
- Traffic Ops ORT (atstccfg)
## What is the best way to verify this PR?
Make sure all the existing tests are still passing.
## If this is a bug fix, what versions of Traffic Control are affected?
- master
- 5.0.0RC1
## The following criteria are ALL met by this PR
- [x] No new tests because no behavior changes
- [x] Documentation is unnecessary
- [x] An update to CHANGELOG.md is not necessary
- [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 merged pull request #5248: Fix servers client methods changing call signatures
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5248:
URL: https://github.com/apache/trafficcontrol/pull/5248
----------------------------------------------------------------
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 commented on a change in pull request #5248: Fix servers client methods changing call signatures
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5248:
URL: https://github.com/apache/trafficcontrol/pull/5248#discussion_r518885076
##########
File path: lib/go-tc/servers.go
##########
@@ -574,11 +577,96 @@ func (s Server) ToNullable() ServerNullableV2 {
}
}
+// ToNonNullable converts the ServerNullableV2 safely to a Server structure.
+func (s ServerNullableV2) ToNonNullable() Server {
+ coerceBool := func(b *bool) bool {
+ if b == nil {
+ return false
+ }
+ return *b
+ }
+ coerceInt := func(i *int) int {
+ if i == nil {
+ return 0
+ }
+ return *i
+ }
+ coerceString := func(s *string) string {
+ if s == nil {
+ return ""
+ }
+ return *s
+ }
Review comment:
These functions should be declared at the package level for reuse. Go intentionally disallows declaring functions within other functions.
##########
File path: lib/go-tc/servers.go
##########
@@ -574,11 +577,96 @@ func (s Server) ToNullable() ServerNullableV2 {
}
}
+// ToNonNullable converts the ServerNullableV2 safely to a Server structure.
+func (s ServerNullableV2) ToNonNullable() Server {
Review comment:
In `atstccfg`, we have `toreq.ServersToNullable()`, which accepts and returns arrays of structs, not just a single one.
https://github.com/apache/trafficcontrol/blob/1512b5ebf367466c60d70b239e680b2e04ee6ac2/traffic_ops_ort/atstccfg/toreq/toreq.go#L162-L164
Should this too be a function that accepts and returns arrays of structs, rather than a method on `ServerNullableV2`?
----------------------------------------------------------------
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 commented on a change in pull request #5248: Fix servers client methods changing call signatures
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5248:
URL: https://github.com/apache/trafficcontrol/pull/5248#discussion_r518896031
##########
File path: lib/go-tc/servers.go
##########
@@ -574,11 +577,96 @@ func (s Server) ToNullable() ServerNullableV2 {
}
}
+// ToNonNullable converts the ServerNullableV2 safely to a Server structure.
+func (s ServerNullableV2) ToNonNullable() Server {
Review comment:
Works for me, and the dot notation is a plus :+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] ocket8888 commented on a change in pull request #5248: Fix servers client methods changing call signatures
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5248:
URL: https://github.com/apache/trafficcontrol/pull/5248#discussion_r518891741
##########
File path: lib/go-tc/servers.go
##########
@@ -574,11 +577,96 @@ func (s Server) ToNullable() ServerNullableV2 {
}
}
+// ToNonNullable converts the ServerNullableV2 safely to a Server structure.
+func (s ServerNullableV2) ToNonNullable() Server {
Review comment:
That just sounds like a stylistic choice. Other server conversion things are methods - like Upgrade and ToNullable - so I made this one as well.
I think I made all of those too, so I think it's fair to say I prefer `server.Method()` over `Method(server)`.
----------------------------------------------------------------
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