rob05c commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-604461371 >it serves as an example of the idiom for future dev. >To me that came across as "all of our Go components should follow this example in the future". That wasn't the intention. >Also includes an example of the latest client getting Delivery >Services, and falling back to the vendored if the TO isn't the latest. >This isn't actually necessary right now, but it's likely to be very >shortly, and it serves as an example of the idiom for future dev. When I said "it serves as an example of the idiom for future dev" I was referring to the phrase "example of the latest client getting Delivery Services, and falling back to the vendored if the TO isn't the latest," specifically these lines: ``` dses, unsupported, err := cfg.TOClientNew.GetCDNDeliveryServices(server.CDNID) if err == nil && unsupported { log.Warnln("Traffic Ops newer than ORT, falling back to previous API Delivery Services!") dses, err = cfg.TOClient.GetCDNDeliveryServices(server.CDNID) } ``` The sentence "it serves as an example of the idiom for future dev" wasn't intended as a broad sweeping declaration, but in context, for this very particular code pattern in ORT, as long as it exists. Or in other words, "I wrote something to show people how to do it, even though it isn't actually necessary yet." >the problem with that is that ORT is not vendoring the corresponding Go structs You are correct: if a developer uses a new feature in the latest version of Traffic Ops in ORT with a `TOClient` call and not a `TOClientNew` call, it will be a runtime error, not a compile-time error. I agree, that isn't desirable. This solution isn't perfect. You are also correct that the cause of this is ORT/atstccfg not vendoring the client structs. The problem is, if we vendor the client structs, they become different symbols than used by `lib/go-atscfg`. Further, it's not possible to write conversion functions (e.g. `VendoredDSToDS()`) because Go doesn't allow naming vendored symbols. We would have to either convert every object in-place, with a huge amount of duplicate and illegible code, or else not use libraries for anything that uses `lib/go-tc`. Again, this solution isn't perfect. There are a number of alternatives, each with their own disadvantages. If there's a perfect solution, I haven't seen it. >maybe the TO Go client should optionally downgrade the request one minor version if it gets back a 501. For instance GET /api/2.1/foos returned a 501, so optionally retry one time with GET /api/2.0/foos. If the retry was successful, set the proper 2.1 defaults and return the result. There are several disadvantages with that. It hides the behavior from apps using the client, and makes it impossible for them to discern between the latest and a client-assumed default. In cases where the client (ORT/TM/etc) needs to discern and act differently than the presumed-default, it can lead to the app doing the wrong thing, without even realizing it. And because the TO client hides it, it becomes impossible to do the right thing. It's also considerably more code in the client; likely more than it would take in actual apps using the client (e.g. ORT only uses a few functions/endpoints). I'm guessing most apps will also need to do their own handling for previous versions, in which case it hasn't saved any code for apps using the TO Client. I'm not saying that solution is bad, just that there are disadvantages, as with this solution. I think both this PR and your suggestion both have potential run-time errors. But this solution seems pretty easy for ORT developers to maintain. It seems unlikely that someone working on ORT won't be aware that the new feature they're adding is new, or if they don't, that they won't test and immediately discover it doesn't work. And this seems relatively easy for ORT developers to work with - it's a few lines to add new features, the extra code goes away as soon as the next release is made, and it's equally easy to refresh the vendored client (even without the helper script I added). For the time being, would this solution be ok, just to make ORT work? And maybe we can rethink the client later?
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
