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] > <javascript:>> 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] <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.
