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