Although I think we could probably make an exception to keep the route at 1.1 with the breaking change of removing the non-streaming option since I would consider the non-streaming option "bad", I can also see us just rewriting it without the non-streaming option as a 2.0 route (what Jeremy proposed as Option 3). That seems to be the most non-controversial option. In that case, we should note in the docs and changelog that the non-streaming option is deprecated and won't be carried forward to 2.0.
- Rawlin On Tue, Dec 10, 2019 at 8:05 AM Jeremy Mitchell <[email protected]> wrote: > > There are definitely lots of downsides to the default implementation > (providing a link to the iso) and that's why i believe the ?stream=true > option was added. Too bad it wasn't always the default/only behavior. > > I only see a few options here: > > 1. rewrite POST /api/1.1/isos?stream=true|false with the current behavior > and then with api 2.x consider a new route that doesn't support the query > param but always streams. > > 2. rewrite the streaming part of POST /api/1.1/isos and then figure out a > way in routes.go to route POST /api/1.1/isos?stream=true to the Go > implementation and allow POST /api/1.1/isos?stream=false (or no stream > query param) to fallback to the Perl implementation and then with api 2.x > consider a new route that doesn't support the query param but always > streams. > > 3. don't rewrite POST /api/1.1/isos?stream=true|false at all and let it > continue to be served by Perl and eventually deprecated. Instead take the > work you have done and write POST /api/2.0/isos that has no query param > support and simply streams the iso. > > Changing the response of stream=false in 1.x to an error message would be a > breaking api change and should probably be avoided and also forcing users > to use the blacklist to honor the 1.x contract of that endpoint might be a > bit much. > > Unfortunately, it feels like we've backed ourselves into a corner with this > one. > > Jeremy > > On Mon, Dec 9, 2019 at 2:51 PM Williams, Adam <[email protected]> > wrote: > > > This concerns the `/isos` API endpoint and the re-write from Perl to Go. > > The endpoint provides users a way to generate and download an ISO [0]. > > Currently it has two modes: > > > > - stream=true: The response is the ISO file that the user receives as a > > download. Nothing is permanently saved on TO servers in this case, instead > > the data is streamed directly to the client. > > - stream=false: The response contains a link to download the ISO file. > > TrafficOps saves the ISO file on the server’s filesystem for a later > > download by the client. > > > > The stream=false mode has a few shortcomings: > > - The generated files will either eventually fill up the server’s disk or > > need to be periodically deleted (breaking the download links). > > - Clients must be routed directly to the TrafficOps server that contains > > the ISO file, which can be complicated in a setup where multiple TrafficOps > > are fronted by a load balancer. It also requires exposing the TrafficOps > > server when otherwise not necessary. > > > > I propose returning a user-friendly error when stream=false, essentially > > restricting users to stream=true. Operators wishing to allow stream=false > > can blacklist the route and force Perl to handle it. Barring objections, > > I’ll include this as part of the re-write from Perl to Go. > > > > [0] > > https://traffic-control-cdn.readthedocs.io/en/latest/api/isos.html#isos > >
