I think for rewriting existing Perl endpoints, we should probably keep the route uncommented in the Perl just for testing/comparison purposes until we're satisfied with it. As a later step, comment the route and make the handler return an error message.
At least for reviewing Go-rewrite PRs, the validation typically requires hitting both the Perl and the Go routes and comparing the results. That becomes more difficult when the Perl route is unavailable. That said, I think we're at that "satisfied" step for a lot of the Go-rewritten routes and should be free to comment out those routes in the Perl that have been rewritten for some time now and make the handlers return errors. - Rawlin On Wed, Aug 7, 2019 at 10:09 AM Jeremy Mitchell <[email protected]> wrote: > > I still think it would just be easiest to comment out unused routes (i.e. > UI routes or 1.0 routes): > > https://github.com/apache/trafficcontrol/blob/master/traffic_ops/app/lib/TrafficOpsRoutes.pm#L70 > https://github.com/apache/trafficcontrol/blob/master/traffic_ops/app/lib/TrafficOpsRoutes.pm#L901 > > ^^ IMO both those sections should have ALL routes commented out by > now...but they don't so we should figure out what it takes to comment the > rest out/disable them. > > Also, as API routes are rewritten to Go, why not comment out the related > route here: > > https://github.com/apache/trafficcontrol/blob/master/traffic_ops/app/lib/TrafficOpsRoutes.pm#L394 > > Then it becomes very clear what is left of the API to convert to Go. > > Oh, one more thing. All "internal" routes should be disabled by now as well. > > Jeremy > > > > On Wed, Aug 7, 2019 at 8:02 AM ocket 8888 <[email protected]> wrote: > > > I don't really have an opinion on "return an error message", but I feel > > strongly that if that's done it should not also delete the rest of the > > handler. Because that'd make rewrites more complicated to both write and > > test. > > > > On Tue, Aug 6, 2019 at 4:28 PM Henderson, Daisy < > > [email protected]> > > wrote: > > > > > I agree with Andy on the 'return an error message' for now, and comment > > > out the already ported Perl endpoints. Is there any issue with simply > > > commenting them out for now? > > > > > > -Daisy > > > > > > On 8/6/19, 3:20 PM, "Schmidt, Andrew (Contractor)" < > > > [email protected]> wrote: > > > > > > In that case I would vote for the conclusion of the original > > > discussion, 'return an error message'. I think it's important to get a > > > release out that actually has perl endpoints disabled so we can find out > > if > > > any of our users are accessing them directly. Clingy legacy code is the > > > worst. > > > > > > On 8/6/19, 3:12 PM, "ocket 8888" <[email protected]> wrote: > > > > > > We don't need to delete it, just commenting out the route for the > > > moment > > > would be enough imo. > > > The Perl libraries could be calling into each other, so deleting > > > them is > > > unsafe, in general. > > > > > > On Tue, Aug 6, 2019 at 1:34 PM Robert Butts <[email protected]> > > > wrote: > > > > > > > @Daisy_Henderson We had a discussion on it, and the consensus > > > was to leave > > > > the route functions in place, and change the functions they > > call > > > to > > > > immediately return an error, "This endpoint should not have > > been > > > possible > > > > to call." > > > > > > > > TLDR, the reason is because Perl is so dynamic, you could > > > concatenate > > > > strings together to make a function name to call, and it would > > be > > > > impossible to find in the code. So, there was a fear we could > > > break things, > > > > and there's no way to be sure. The counterargument being: the > > > old, wrong > > > > code would break things worse if it were ever called. > > > > > > > > Thread is here: > > > > > > > > > > > > > https://lists.apache.org/thread.html/12cad3baa9af9f92e47772f3cd7be22514ddc899d5ba18f9e35d5e1c@%3Cdev.trafficcontrol.apache.org%3E > > > > > > > > Nobody ever did it, but it shouldn't be hard to do. That said, > > > there's > > > > nothing preventing us from voting again, if we think the > > > consensus will be > > > > different now. > > > > > > > > I'm personally +1 on just deleting the routes and functions, if > > > we're > > > > re-voting on it. > > > > > > > > > > > > On Tue, Aug 6, 2019 at 1:19 PM Henderson, Daisy < > > > > [email protected]> > > > > wrote: > > > > > > > > > Chiming in here. For the Perl API routes that have already > > > been rewritten > > > > > in Go, can we comment those out in the 'TrafficOpsRoutes.pm' > > > file? It > > > > would > > > > > help organize the non-ported endpoints and make it more > > > visible as to > > > > which > > > > > Perl endpoints are still being used. > > > > > > > > > > -Daisy > > > > > > > > > > On 8/6/19, 8:46 AM, "Robert Butts" <[email protected]> wrote: > > > > > > > > > > >There definitely are some that are still defined and not > > > > > commented-out in > > > > > the Perl routes file. I haven't really thought about what > > > to do with > > > > > those > > > > > - maybe I'll just open an issue for each that says > > > something like > > > > > "Decide > > > > > what to do about {{route}}" and then deal with it later. > > > Thoughts? > > > > > > > > > > If they aren't under /api, they don't fall under the API > > > promise, but > > > > > they > > > > > do fall under the project "deprecate-and-remove" "one > > > major version" > > > > > promise. I _think_ we declared all non-API versions > > > deprecated a long > > > > > time > > > > > ago, but I'm not positive. We should probably declare it > > > again, just > > > > > to be > > > > > sure. Either on the list, or with comments in the code, > > or > > > both. If > > > > we > > > > > can > > > > > find that declaration, that would be great, then we can > > > remove them > > > > in > > > > > 4.0. > > > > > > > > > > Then, if they aren't used inside TC, they can just be > > > removed. If > > > > they > > > > > are, > > > > > we'll need to rewrite them under /api, and change > > whatever > > > scripts or > > > > > apps > > > > > are using them. I already did this for some, several some > > > months ago, > > > > > but > > > > > I'm not sure if I got all of them. It's hard to be > > > certain. We should > > > > > both > > > > > search the code for anything calling them, and check our > > > production > > > > TO > > > > > access logs. > > > > > > > > > > > > > > > On Tue, Aug 6, 2019 at 8:29 AM ocket 8888 < > > > [email protected]> > > > > wrote: > > > > > > > > > > > Yeah, I'll need to do some digging to find an > > exhaustive > > > list, but > > > > > yours is > > > > > > a good starting point. > > > > > > > > > > > > > There may be several non-API routes which are still > > > used by > > > > Traffic > > > > > > Control > > > > > > > > > > > > There definitely are some that are still defined and > > not > > > > > commented-out in > > > > > > the Perl routes file. I haven't really thought about > > > what to do > > > > with > > > > > those > > > > > > - maybe I'll just open an issue for each that says > > > something like > > > > > "Decide > > > > > > what to do about {{route}}" and then deal with it > > later. > > > Thoughts? > > > > > > > > > > > > On Tue, Aug 6, 2019 at 8:26 AM Robert Butts < > > > [email protected]> > > > > wrote: > > > > > > > > > > > > > Just to check, you all know this isn't a list of all > > > endpoints in > > > > > Traffic > > > > > > > Ops, right? As the issue says, "all tables queried > > for > > > the > > > > > CRConfig." > > > > > > > > > > > > > > That list, which really existed for my own use for > > > Self Service, > > > > > is only > > > > > > > the endpoints which query tables used in the > > CRConfig, > > > and not > > > > > including > > > > > > > ATS configs. It's a lot of endpoints, but not all of > > > them. If you > > > > > want a > > > > > > > list of all of them, you'll have to compare > > > TrafficOpsRoutes.pm > > > > and > > > > > > > routes.go. > > > > > > > > > > > > > > Also be aware, for the rewrite, there may be several > > > non-API > > > > > routes which > > > > > > > are still used by Traffic Control, and will have to > > be > > > rewritten > > > > > as new > > > > > > > /api endpoints before Perl can be removed. > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 6, 2019 at 8:15 AM ocket 8888 < > > > [email protected]> > > > > > wrote: > > > > > > > > > > > > > > > Yeah, that's the plan > > > > > > > > > > > > > > > > On Tue, Aug 6, 2019 at 8:08 AM Jeremy Mitchell < > > > > > [email protected]> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Just so we're clear on what your asking. You want > > > to create a > > > > > github > > > > > > > > issue > > > > > > > > > for each unchecked item in this issue, right? > > > > > > > > > > > > > > > > > > > > > > > https://protect2.fireeye.com/url?k=e1de0ea2-bd3a0069-e1de2916-000babff3540-b55aca537864275e&u=https://github.com/apache/trafficcontrol/issues/2232 > > > > > > > > > > > > > > > > > > Then what? close 2232? Each endpoint can be > > tackled > > > > > individually so > > > > > > > that > > > > > > > > > seems to make sense to me. > > > > > > > > > > > > > > > > > > Jeremy > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 6, 2019 at 7:11 AM ocket 8888 < > > > > [email protected] > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > So it seems that at least we can all agree > > these > > > should > > > > have > > > > > their > > > > > > > own > > > > > > > > > > issues, so far. I'll leave the question of > > > > > label/milestone/project > > > > > > > > > > unanswered for now and just start opening > > issues. > > > > > > > > > > > > > > > > > > > > On Mon, Aug 5, 2019 at 7:49 PM Jeremy Mitchell > > < > > > > > > > [email protected]> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > I think the `TO API Golang Rewrite` actually > > > makes > > > > perfect > > > > > sense > > > > > > > as a > > > > > > > > > > > milestone. It will truly be "milestone" when > > > we finish > > > > > that. I'm > > > > > > > good > > > > > > > > > if > > > > > > > > > > > others are (or are not opposed). > > > > > > > > > > > > > > > > > > > > > > On Mon, Aug 5, 2019 at 3:58 PM ocket8888 < > > > > > [email protected]> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Huh. That's new. btw, I don't think you can > > > send images > > > > > through > > > > > > > the > > > > > > > > > > > > mailing list > > > > > > > > > > > > > > > > > > > > > > > > I still think a project is overkill, as it > > > depends on > > > > > assigning > > > > > > > > > things > > > > > > > > > > > > to people, tracking things that are "In > > > Progress" and > > > > > whatnot. > > > > > > It > > > > > > > > > > > > doesn't give you anything that a milestone > > > wouldn't. > > > > > > > > > > > > > > > > > > > > > > > > There's no reason a milestone must > > > correspond to some > > > > > release. > > > > > > > > > > > > > > > > > > > > > > > > On 8/5/19 3:51 PM, Jeremy Mitchell wrote: > > > > > > > > > > > > > image.png > > > > > > > > > > > > > looks like a project "tracks absolute > > > progress toward > > > > > some > > > > > > > goal". > > > > > > > > > > it's > > > > > > > > > > > > > got a progress bar and everything: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://protect2.fireeye.com/url?k=31132953-6df72798-31130ee7-000babff3540-9ff47b40213aec0f&u=https://github.com/apache/trafficcontrol/projects/2 > > > > > > > > > > > > > > > > > > > > > > > > > > > I wouldn't be opposed to creating a > > > project as > > > > well, > > > > > but > > > > > > I'm > > > > > > > > > > > skeptical > > > > > > > > > > > > > that it would be used. > > > > > > > > > > > > > > > > > > > > > > > > > > It would be used if you managed it and > > > made sure it > > > > > stayed up > > > > > > > to > > > > > > > > > > date. > > > > > > > > > > > > > You can also set up projects to > > > auto-update. When you > > > > > create > > > > > > > the > > > > > > > > > > > > > project, select the type. for example: > > > > > > > > > > > > > > > > > > > > > > > > > > /Automated kanban > > > > > > > > > > > > > Kanban-style board with built-in triggers > > > to > > > > > automatically > > > > > > move > > > > > > > > > > issues > > > > > > > > > > > > > and pull requests across To do, In > > > progress and Done > > > > > columns. > > > > > > > > > > > > > / > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Aug 5, 2019 at 3:20 PM ocket8888 > > < > > > > > > [email protected] > > > > > > > > > > > > > <mailto:[email protected]>> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > A project is a way of organizing work > > > by keeping > > > > > track of > > > > > > > who > > > > > > > > > is > > > > > > > > > > > > > working > > > > > > > > > > > > > on what and at what stage a > > particular > > > part of > > > > the > > > > > > project > > > > > > > > is. > > > > > > > > > A > > > > > > > > > > > > > milestone tracks absolute progress > > > toward some > > > > > goal, > > > > > > which > > > > > > > is > > > > > > > > > all > > > > > > > > > > > I'm > > > > > > > > > > > > > seeking to accomplish. > > > > > > > > > > > > > > > > > > > > > > > > > > I wouldn't be opposed to creating a > > > project as > > > > > well, but > > > > > > > I'm > > > > > > > > > > > > > skeptical > > > > > > > > > > > > > that it would be used. > > > > > > > > > > > > > > > > > > > > > > > > > > On 8/5/19 3:02 PM, Jeremy Mitchell > > > wrote: > > > > > > > > > > > > > > I've just always associated > > > milestone with > > > > > release for > > > > > > > some > > > > > > > > > > > > > reason... > > > > > > > > > > > > > > > > > > > > > > > > > > > > Feels like a "project" to me > > > however. Oh look > > > > at > > > > > that > > > > > > > there > > > > > > > > > was > > > > > > > > > > > > > one for > > > > > > > > > > > > > > this at one time: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://protect2.fireeye.com/url?k=8449032a-d8ad0de1-8449249e-000babff3540-ab0e9732ae0e32f0&u=https://github.com/apache/trafficcontrol/projects?query=is%3Aclosed > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Aug 5, 2019 at 2:56 PM > > > ocket8888 < > > > > > > > > > [email protected] > > > > > > > > > > > > > <mailto:[email protected]>> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > >> As far as I know, the only thing > > > you get with > > > > a > > > > > > > Milestone > > > > > > > > > > > > > (immediately > > > > > > > > > > > > > >> and by default) that doesn't come > > > with a label > > > > > is the > > > > > > > > > ability > > > > > > > > > > > > > to track > > > > > > > > > > > > > >> progress towards reaching that > > > milestone. > > > > Which > > > > > I > > > > > > think > > > > > > > is > > > > > > > > > > > > > appropriate > > > > > > > > > > > > > >> here - and is actually all of my > > > motivation > > > > for > > > > > > > > suggesting a > > > > > > > > > > > > > milestone > > > > > > > > > > > > > >> rather than a label. What > > "baggage" > > > do you > > > > mean? > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> On 8/5/19 2:52 PM, Chris Lemmons > > > wrote: > > > > > > > > > > > > > >>> I agree that separate tickets is > > > ideal. Would > > > > > a tag > > > > > > > work > > > > > > > > > for > > > > > > > > > > > this > > > > > > > > > > > > > >>> purpose? Milestones have other > > > baggage that > > > > > doesn't > > > > > > > apply > > > > > > > > > > here. > > > > > > > > > > > > > >>> > > > > > > > > > > > > > >>> On Mon, Aug 5, 2019 at 2:16 PM > > > ocket 8888 < > > > > > > > > > > [email protected] > > > > > > > > > > > > > <mailto:[email protected]>> wrote: > > > > > > > > > > > > > >>>> Currently to see what endpoints > > > are > > > > rewritten > > > > > and > > > > > > > which > > > > > > > > > > still > > > > > > > > > > > > > need to be > > > > > > > > > > > > > >>>> done, you need to check out this > > > one, > > > > > specific issue > > > > > > > Rob > > > > > > > > > > made > > > > > > > > > > > > > forever > > > > > > > > > > > > > >> ago: > > > > > > > > > > > > > >>>> > > > > > > > > > > > > > > > > > > > > https://protect2.fireeye.com/url?k=755603ef-29b20d24-7556245b-000babff3540-00731ec0bbe9a19e&u=https://github.com/apache/trafficcontrol/issues/2232 > > > > > > > > > > > > > >>>> I think it'd be better to give > > > each endpoint > > > > > its own > > > > > > > > > Issue - > > > > > > > > > > > > > only the > > > > > > > > > > > > > >> ones > > > > > > > > > > > > > >>>> that still need to be rewritten > > - > > > and then > > > > > link them > > > > > > > all > > > > > > > > > in > > > > > > > > > > a > > > > > > > > > > > > "Go > > > > > > > > > > > > > >> Rewrite" > > > > > > > > > > > > > >>>> milestone. It's much easier to > > > organize. > > > > Then > > > > > we can > > > > > > > > stop > > > > > > > > > > > > > re-building > > > > > > > > > > > > > >> this > > > > > > > > > > > > > >>>> list. > > > > > > > > > > > > > >>>> I volunteer to comb through the > > > list and > > > > > figure out > > > > > > > what > > > > > > > > > > > > > endpoints > > > > > > > > > > > > > >> actually > > > > > > > > > > > > > >>>> are rewritten and do the work of > > > creating > > > > > issues for > > > > > > > > them, > > > > > > > > > > > > > provided > > > > > > > > > > > > > >> that's > > > > > > > > > > > > > >>>> acceptable to everyone? I think > > > we typically > > > > > only > > > > > > use > > > > > > > > > > > > > milestones for > > > > > > > > > > > > > >>>> releases, so I thought I should > > > check. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
