On 14 February 2017 at 04:41, Steve Armstrong <[email protected]> wrote:
> Now it's me who's missing something. > > 1. I thought we couldn't modify the Request object, so if we want to strip > the prefix before sending a Reqeust to the mux, we need to clone or > otherwise create a new one. Is that what you're proposing? > I think Brad's objection was because I was doing that on the app handler, which is a proxy. I am now doing it on the scanning cabinet app itself, which is the final recipient of the request, where I'd say we can do whatever we want with the request since it has reached its destination. > 2. I looked at your new patch, and I can't seem to find where you're > stripping the prefix off to see how you did it. > https://camlistore-review.googlesource.com/#/c/5416/12/app/scanningcabinet/handler.go@171 As an aside, I've been keeping my scancab branch at https://github.com/ > stevearm/camlistore/tree/scancab until it was ready to post to your CL. > It's got your rebased commit and 3 small fixes (one of which you've already > got in your new rebase), then 3 feature commits I was going to propose once > the base scancab landed. > Once we're past the prefix issue, and once at least one person (so, most likely you) has reviewed and +1ed the scancab CL, I'll land it. Then https://camlistore-review.googlesource.com/9206 . Then we can move on to whatever changes you propose. On Monday, February 13, 2017 at 2:57:56 PM UTC-8, mpl wrote: >> >> Hi, >> >> I wanted to test your CL (https://camlistore-review.googlesource.com/9206) >> in conjunction with the scanning cabinet, and I found out that we don't >> need CAMLI_APP_URL_PREFIX after all, if we strip the prefix from the >> request's URL Path right before giving it to the muxer. PTAL at >> https://camlistore-review.googlesource.com/#/c/5416/12 , and let me know >> what you think or if I'm missing something again. >> >> thanks. >> >> >> On 6 February 2017 at 20:09, Steve Armstrong <[email protected]> >> wrote: >> >>> Great, thanks. I'll implement that then as a CL (adding >>> CAMLI_APP_URL_PREFIX) and rebase my scanningcabinet branch on top for >>> submission. >>> >>> On Monday, February 6, 2017 at 7:15:00 AM UTC-8, mpl wrote: >>> >>>> >>>> >>>> On 3 February 2017 at 08:10, Steve Armstrong <[email protected]> >>>> wrote: >>>> >>>>> On Monday, January 30, 2017 at 8:00:28 AM UTC-8, mpl wrote: >>>>>> >>>>>> >>>>>> My base concern is /scancab/ is user defined. >>>>>>> >>>>>> >>>>>> Why does that concern you? >>>>>> >>>>> >>>>> Because it could be anything, so scanningcabinet needs to be told what >>>>> it is so it can strip it out (either in env vars or in the callback >>>>> config). >>>>> >>>>>> >>>>>> >>>>>>> Either camlistore should remove it from the url before proxying, or >>>>>>> should supply it in the env vars when starting the app so the app knows >>>>>>> what it needs to remove. >>>>>>> >>>>>> >>>>>> Why can't you use camlistore.org/pkg/app.PathPrefix , to know what >>>>>> needs stripping, just like the publisher does, and like the scanning >>>>>> cabinet handler already does too actually? What does not work in doing >>>>>> that? >>>>>> >>>>> >>>>> From what I can tell, scanningcabinet just uses app.PathPrefix to get >>>>> the base url when making links. Since this function only works from within >>>>> a request, I can't bind the prefixed urls to the ServeMux at startup. I >>>>> also can't modify the Request object and feed it back into the ServeMux >>>>> per >>>>> our previous discussion about Camlistore removing the prefix in the first >>>>> place. Prefix is also not provided by default in the config returned by >>>>> the >>>>> app config callback url. I'm unsure which of these things I should be >>>>> fixing in order to get around this, or am I missing something critical? >>>>> >>>> >>>> No, I think you're right; we can't set up the muxer's handlers if we >>>> don't know the prefix path. >>>> The publisher circumvents this problem by muxing everything to "/", >>>> stripping the request url path of the prefix, and dispatching manually to >>>> various handlers, which I think is not a good idea. The scanning cabinet >>>> was on the back burner when we fixed the app handler to not mutate the >>>> request url path, which is why I didn't think to raise that objection at >>>> the time. >>>> So yes, the app handler should probably provide to the app the prefix >>>> path on which it is mounted on Camlistore. The most obvious/clean way would >>>> be to add another env var to doc/app-environment.md, that the app >>>> handler is then contractually obliged to provide to the app when it creates >>>> the app's process. I actually went back and forth in the various CLs as to >>>> whether we should include the prefix in an env var. >>>> Another way, if we think that the prefix is not always necessary (but I >>>> think it pretty much is), is to include it in the appConfig that the all >>>> can optionally request from the app handler on startup. >>>> >>>> >>>>> >>>>>> >>>>>> On Wednesday, January 25, 2017 at 5:37:32 AM UTC-8, mpl wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 25 January 2017 at 09:06, Steve Armstrong <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> My suggestion to have Camlistore drop the handler prefix from the >>>>>>>>> url before it proxies through to the app was rejected because a design >>>>>>>>> decision was already made: https://github.com/camli >>>>>>>>> store/camlistore/issues/833 >>>>>>>>> >>>>>>>> >>>>>>>> Is there a conversation somewhere why this is so? >>>>>>>>> >>>>>>>> >>>>>>>> https://camlistore-review.googlesource.com/#/c/6916/5/pkg/ >>>>>>>> server/app/app.go@102 >>>>>>>> So I suppose it must come from some RFC somewhere about proxying... >>>>>>>> >>>>>>>> Are we planning on having the prefix in an environment variable so >>>>>>>>> the app always knows what the url will be and it can handle it? While >>>>>>>>> I'm >>>>>>>>> modifying the scanningcabinet to work with this, I need to go down >>>>>>>>> one of >>>>>>>>> these two paths (Camlistore takes care of it, or passes all required >>>>>>>>> info >>>>>>>>> to the app so it can do it) and I don't want to implement opposite to >>>>>>>>> something that's already been worked out by the team. >>>>>>>>> >>>>>>>> >>>>>>>> I was trimming the prefix because I thought there was no good >>>>>>>> reason for the app to be burdened with the knowledge of the app >>>>>>>> handler's >>>>>>>> prefix (i.e. I thought it was more elegant like that), but there was no >>>>>>>> technical problem solved by doing so, or no good technical reason for >>>>>>>> doing >>>>>>>> it. So I'm having some trouble understanding what the issue you're >>>>>>>> trying >>>>>>>> to address is. What is it in the scanningcabinet that does not work and >>>>>>>> that you're trying to fix? >>>>>>>> >>>>>>>> Or are you just asking about the existence of something like >>>>>>>> camlistore.org/pkg/app.PathPrefix ? >>>>>>>> >>>>>>>> >>>>>>>>> Thanks >>>>>>>>> >>>>>>>>> On Monday, January 23, 2017 at 9:28:03 AM UTC-8, mpl wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 22 January 2017 at 20:46, Steve Armstrong < >>>>>>>>>> [email protected]> wrote: >>>>>>>>>> >>>>>>>>>>> Posted separate review so Camlistore, when getting a request for >>>>>>>>>>> an app, will strip off the app's prefix from the url before >>>>>>>>>>> prepending the >>>>>>>>>>> app's host url: https://camlistore-review.googlesource.com/9207 >>>>>>>>>>> >>>>>>>>>>> Finally, what is the policy for updating someone else's review? >>>>>>>>>>> I've got mpl's scancab review (https://camlistore-review.goo >>>>>>>>>>> glesource.com/#/c/5416/) rebased onto ToT and working (here on >>>>>>>>>>> my Github fork https://github.com/stevea >>>>>>>>>>> rm/camlistore/tree/scancab). Should I post a separate review >>>>>>>>>>> with mpl's changes and my two small commits squashed into a single >>>>>>>>>>> commit, >>>>>>>>>>> or should I be updating the existing review? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> The main reason my CL isn't up to date is because the main >>>>>>>>>> interested party (Brad) does not have time to review it, and because >>>>>>>>>> it's >>>>>>>>>> not in our first priorities. Also because obviously I wouldn't merge >>>>>>>>>> it >>>>>>>>>> without review. >>>>>>>>>> That said, if it gets reviewed and tested enough by others, it is >>>>>>>>>> more acceptable for me to land it without waiting for Brad. >>>>>>>>>> So yes, if you want you can add a patchset on top of the existing >>>>>>>>>> CL, and we can move forward from there if there's enough interest. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> >>>>>>>>>>> On Sunday, January 22, 2017 at 11:12:23 AM UTC-8, Steve >>>>>>>>>>> Armstrong wrote: >>>>>>>>>>>> >>>>>>>>>>>> I've posted the following to setup defaults for the keys I >>>>>>>>>>>> thought should be optional: https://camlistore-r >>>>>>>>>>>> eview.googlesource.com/#/c/9206/ >>>>>>>>>>>> >>>>>>>>>>>> On Sunday, January 22, 2017 at 9:32:50 AM UTC-8, Steve >>>>>>>>>>>> Armstrong wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> I'm trying to get the scanningcabinet working ( >>>>>>>>>>>>> https://camlistore-review.googlesource.com/#/c/5416/). I've >>>>>>>>>>>>> rebased it up on ToT and it seems to be functional, but I'm >>>>>>>>>>>>> having a lot of >>>>>>>>>>>>> confusion with the app interface with camlistore. >>>>>>>>>>>>> >>>>>>>>>>>>> I added the following to my config: >>>>>>>>>>>>> >>>>>>>>>>>>> "/scancab/": { >>>>>>>>>>>>> "handler": "app", >>>>>>>>>>>>> "enabled": true, >>>>>>>>>>>>> "handlerArgs": { >>>>>>>>>>>>> "prefix": "/scancab/", >>>>>>>>>>>>> "program": "scanningcabinet", >>>>>>>>>>>>> "listen": "127.0.0.1:2055", >>>>>>>>>>>>> "backendURL": "http://127.0.0.1:2055/"; >>>>>>>>>>>>> } >>>>>>>>>>>>> }, >>>>>>>>>>>>> >>>>>>>>>>>>> I'd like to drop as much redundant info as possible. Why is >>>>>>>>>>>>> "prefix" required twice? Can we plumb through the "/scancab/" key >>>>>>>>>>>>> down into >>>>>>>>>>>>> the handler's constructor so it has it as a default? This key >>>>>>>>>>>>> seems to be >>>>>>>>>>>>> how scannincabinet would call back to Camlistore. Shouldn't that >>>>>>>>>>>>> not be >>>>>>>>>>>>> configurable and just be a randomly generated URL that's passed >>>>>>>>>>>>> into >>>>>>>>>>>>> scanningcabinet when it's run (like the auth is)? >>>>>>>>>>>>> >>>>>>>>>>>>> Also, "listen" or "serverListed" are required. Can't we just >>>>>>>>>>>>> default to a random port added to Camlistore's ip if neither one >>>>>>>>>>>>> is >>>>>>>>>>>>> specified? >>>>>>>>>>>>> >>>>>>>>>>>>> "backendURL" or "serverBaseURL" is also required. Shouldn't it >>>>>>>>>>>>> just default to whatever "listen" defaults to? >>>>>>>>>>>>> >>>>>>>>>>>>> Finally, when I go to http://localhost:3179/scancab/test it >>>>>>>>>>>>> routes to http://localhost:2055/scancab/test. I'd expect it >>>>>>>>>>>>> to trim off the "/scancab/" prefix and route to >>>>>>>>>>>>> http://localhost:2055/test. >>>>>>>>>>>>> >>>>>>>>>>>>> I'm probably missing the goal of how this contract is supposed >>>>>>>>>>>>> to work, and which knobs are intended to be configurable. Could >>>>>>>>>>>>> someone >>>>>>>>>>>>> help me out, as I don't want to submit a pull request that >>>>>>>>>>>>> removes/changes >>>>>>>>>>>>> a bunch of these keys and behaviours if they were intended. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks >>>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>> You received this message because you are subscribed to the >>>>>>>>>>> Google Groups "Camlistore" group. >>>>>>>>>>> To unsubscribe from this group and stop receiving emails from >>>>>>>>>>> it, send an email to [email protected]. >>>>>>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>> You received this message because you are subscribed to the Google >>>>>>>>> Groups "Camlistore" group. >>>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>>> send an email to [email protected]. >>>>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "Camlistore" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to [email protected]. >>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>> >>>>>> >>>>>> -- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "Camlistore" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to [email protected]. >>>>> For more options, visit https://groups.google.com/d/optout. >>>>> >>>> >>>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Camlistore" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> -- > You received this message because you are subscribed to the Google Groups > "Camlistore" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Camlistore" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
