>How do you feel about that? Would that be simple enough? possibly. Make a fork and open a Draft PR so we can see it in action. Otherwise I'm just finding it hard to visualize what the codebase is going to look like with this library in it. ________________________________________ From: Robert Butts <r...@apache.org> Sent: Thursday, April 18, 2019 3:22 PM To: dev@trafficcontrol.apache.org Subject: Re: [EXTERNAL] Re: Traffic Ops API versioning issues
>This is about simplifying our code in the API @jeff.elsloo That's what the tag solution I proposed does. The only difference from not versioning, is that fields will have a new tag, "NewField *int `json:"newField, db:"new_field", api:"1.5"`, and endpoints will have an extra line, "json := api.NewJSON("1.4")". That's it. That would be the entirety of the API code (or very nearly, Rawlin is right, I haven't implemented it to be 100% sure). The library itself is also tiny, it's ~250 lines of logic in a single file. 500 lines including comments and boilerplate. How do you feel about that? Would that be simple enough? On Thu, Apr 18, 2019 at 3:15 PM Fieck, Brennan <brennan_fi...@comcast.com> wrote: > >If you're deploying the head of master, API minor versioning doesn't > really solve that consistent API problem unless we start saying that > every single new field added to an API endpoint is a new minor version > instead of just incrementing an API's version once per TC release. > > Yeah, you shouldn't expect an active development branch to be stable - > it's the whole reason we have releases at all. We can't support something > that changes potentially a dozen times in a day. > > >If someone goes > to the trouble to understand how our APIs work and develops their own > client code, why is it so unreasonable to expect them to also > understand how an update of Traffic Ops could impact their _custom_ > tooling? > > I agree with this so hard. I'd love to just say "TO vX uses the vX API, > major changes to the biggest TC component are a major change to TC", but at > any given time we support and provide bug/security fixes for versions X and > X-1. I'll settle for eliminating minor API versions, though. Developers can > be expected to understand that changing versions of a thing can change > aspects of the ways in which you can interact with said thing. A major > version change means major changes and a minor version change means minor > changes. > ________________________________________ > From: Jeff Elsloo <jeff.els...@gmail.com> > Sent: Thursday, April 18, 2019 2:59 PM > To: dev@trafficcontrol.apache.org > Subject: Re: [EXTERNAL] Re: Traffic Ops API versioning issues > > > Maybe I'm the only one, and everyone else can vote me out, but I don't > see > that as acceptable. It's our responsibility as developers to create a safe > user experience, and unacceptable to declare real bugs to be the user's > fault for not using it right. When our Production CDN goes down because an > Ops person used an old client and didn't "just recompile," it's not that > Ops person's fault, it's our fault as Developers, for designing a dangerous > system. Our job is to prevent the CDN from going down, not to shift the > blame when it does. > > I don't think this is about shifting blame, safety, or the potential > to crash a CDN. This is about simplifying our code in the API and > making it more maintainable. If we simplify the API, we can accelerate > development and get more things done, and maybe even complete this > Golang migration. Another plus is simplification of routes by > eliminating versioning means less code and likely more stability and > safety, easier testing, and less developer confusion, in the long run. > > I think it's unreasonable for us to shoulder the burden and cost to > maintain various API versions because we're afraid we might break some > client out in the wild that might or might not exist. If someone goes > to the trouble to understand how our APIs work and develops their own > client code, why is it so unreasonable to expect them to also > understand how an update of Traffic Ops could impact their _custom_ > tooling? > > Obviously we have to hold up our end of the deal and have good API > documentation and change logs. I think the cost of maintaining that is > much less than API versioning given our experience, especially after > we simplify the APIs. We're already doing much of that today. > -- > Thanks, > Jeff > > On Thu, Apr 18, 2019 at 11:37 AM Robert Butts <r...@apache.org> wrote: > > > > >Without minor versions, #3497 would not even an issue. It's only an > issue > > because of the attempt to support minor versioning. > > > > That's simply not true. It's exactly the same issue. Removing minor > > versioning just hides the issue. You have declared: > > > > >only certain clients that don't handle new unknown fields would > > potentially be broken > > > > >all the client has to do is recompile > > > > Something doesn't cease to be an issue, because you redefine it to be the > > user's fault. It's exactly the same issue, removing minor versions just > > makes it much more difficult to debug. > > > > You're proposing not only removing minor versions, but creating data loss > > and version mismatch bugs, and declaring them to be the user's fault. > > > > Maybe I'm the only one, and everyone else can vote me out, but I don't > see > > that as acceptable. It's our responsibility as developers to create a > safe > > user experience, and unacceptable to declare real bugs to be the user's > > fault for not using it right. When our Production CDN goes down because > an > > Ops person used an old client and didn't "just recompile," it's not that > > Ops person's fault, it's our fault as Developers, for designing a > dangerous > > system. Our job is to prevent the CDN from going down, not to shift the > > blame when it does. > > > > >Switching all the endpoints over to your "apiver" library would not be > as > > trivial to implement or remove as you make it sound. > > > > Maybe. I'm offering to do it. If you're sure, why don't you let me > > demonstrate, and prove myself wrong? > > > > >It would require lots of added API test coverage > > > > Require? That would be ideal, but we have supported minor versions for > the > > history of Traffic Ops, and never had extensive version tests. I agree we > > should, but you're adding additional requirements to further your > position, > > which doesn't seem fair. Notwithstanding, the tag library already has 90% > > test coverage and 3x as many lines of test code as logic; and the API > Tests > > are actually pretty easy, I just added one in the old-version-update fix, > > and it was much easier than I expected: > > > https://github.com/apache/trafficcontrol/pull/3500/commits/16f2c96f086836f1d655fd62e673ee0a5e95e785 > > . > > > > >Certain UPDATE queries might be easy to generate from a given struct if > > the struct only uses a single table, but I don't think something like > that > > would work for a field like `cachegroup.LocalizationMethods` which > doesn't > > come from the cachegroups table > > > > I believe it is easy. The function to parse tags can use the tags in the > > primary object (Cachegroup), and the sub-objects (LocalizationMethods) > will > > have their own version tags. I could be mistaken, I haven't actually > > written the code yet, but I'm pretty sure sub-objects with sub-updates > > won't be any more difficult or require much if any special logic. > > > > > > On Thu, Apr 18, 2019 at 10:37 AM Gray, Jonathan < > jonathan_g...@comcast.com> > > wrote: > > > > > At the end of the day, what I want is a consistent API that I can code > > > against in the head of master that's treated like a contract. As an > API > > > user outside of the ATC repo it's incredibly frustrating to have my > stuff > > > break all the time. It basically encourages never developing using the > > > latest API versions (regardless of how they're defined and even then > things > > > still break retroactively) or a non-official OSS release alltogether. > It's > > > a catch22 to be forced to either not vendor the go/python/bash > libraries > > > which leads to constant develop/recompile/deploys in lockstep with ATC > or > > > vendor and still have to do these things when stuff breaks anyway in > the > > > API. Really debating the native client libraries at all is just a red > > > herring because the root issue is the HTTP API itself which is the real > > > thing to care about since not all integrations use one of the client > > > libraries, nor can be forced to do so, and may require a rigid API > > > definition. > > > > > > Jonathan G > > > > > > > > > On 4/18/19, 10:12 AM, "Rawlin Peters" <rawlin.pet...@gmail.com> > wrote: > > > > > > > The UPDATE statements need modified to fix #3497 even if we get > rid > > > of > > > > versioning. Unless we decide to permanently break all clients > older > > > than > > > > the newest server field, with every new server upgrade. The only > > > other > > > > option is to fix the updates. Unless you know of a way to fix > missing > > > > fields without changing the update statements, that I'm not > seeing? > > > > > > By removing minor versioning, only certain clients that don't > handle > > > new unknown fields would potentially be broken, and I believe only > the > > > TO Go client has that problem in our repo. However, the TO Go > client > > > happens to use the same Go structs as traffic_ops_golang, so > whenever > > > new fields are added to the API, all the client has to do is > recompile > > > with the up-to-date structs. Unless we made breaking changes to the > > > client, in most cases all that would be needed for those clients > is a > > > recompile. Traffic Portal, the Python TO client, and I'm pretty > sure > > > the Java TO client all handle unknown fields properly. > > > > > > Without minor versions, #3497 would not even an issue. It's only an > > > issue because of the attempt to support minor versioning. If we > just > > > support the major version, all client requests would be treated as > v1, > > > and there would only ever be one SQL UPDATE statement per major > > > version. We wouldn't need to "upgrade" 1.2 requests into a 1.4 > struct > > > (thus preventing the bug in #3497) by selecting and inserting all > 1.4 > > > values from the DB into the struct before handling the request or > > > dynamically generating the SQL UPDATE statement to use based on the > > > requested minor version. > > > > > > > So, this solution actually gives us > > > > this bug fix almost for free. All that's required is another > small > > > function > > > > to iterate over the object fields to create the update query. > It's > > > by far > > > > the easiest and simplest fix for #3497; unless we also > permanently > > > break > > > > all older clients on every server upgrade along with the minor > > > version > > > > removal. > > > > > > Switching all the endpoints over to your "apiver" library would > not be > > > as trivial to implement or remove as you make it sound. It would > > > require lots of added API test coverage and a non-trivial amount of > > > code modifications to all API endpoints. Certain UPDATE queries > might > > > be easy to generate from a given struct if the struct only uses a > > > single table, but I don't think something like that would work for > a > > > field like `cachegroup.LocalizationMethods` which doesn't come from > > > the cachegroups table and is updated separately from the rest of > the > > > cachegroup fields. > > > > > > - Rawlin > > > > > > > > > >