Yes, given that the parameters aren't in the query string I think Jeremy's option 3 makes the most sense. Trying to emulate that sort of sounds like more trouble than it's worth.
On Tue, Dec 10, 2019 at 10:58 AM Rawlin Peters <[email protected]> wrote: > 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 > > > >
