On 26 January 2017 at 05:53, Steve Armstrong <[email protected]> wrote:
> > https://camlistore-review.googlesource.com/#/c/6916/5/pkg/ > server/app/app.go@102 > > This comment reads more like modifying the request is not allowed by the > contract of ServeHTTP, not that it's a design issue for camlistore's app > interface contract. It looks like Brad approved the review ( > https://camlistore-review.googlesource.com/#/c/8147/) though, so am I > reading this wrong? > Yes, I took that comment as "this is bad practice for any implementation of ServeHTTP", which is why I reverted it later. I suppose Brad let it in because he knows I'm reliable when it comes to keeping track of comments/remarks and that I would eventually take care of fixing it. You'd have to ask him directly to be sure though. The way my rebase of scanningcabinet works now, it doesn't respond > correctly to /scancab/uploadurl, only to /uploadurl. I'll have to change > this to prefix it properly. That prefix should be supplied by calmistore > though, since it's user-driven from server-config.json, so the user could > set it to whatever. If in the future we end up with a lot of apps (or third > party) and you want to run two apps who both expect to be mounted to > "/picturetool" then when you remap one, that new mapping should be > auto-injected. > > My base concern is /scancab/ is user defined. > Why does that concern you? > 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? 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.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.
