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? 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.
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. 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] > <javascript:>> 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/camlistore/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.googlesource.com/#/c/5416/) rebased >>>>>>>>>> onto ToT and working (here on my Github fork >>>>>>>>>> https://github.com/stevearm/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-review.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] <javascript:>. >> 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.
