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/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-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.
