I tried out your new patch, and removed my prefixing code from my patch, and they work together. Good call on just modifying the Request internally. I updated my review to have just the app-default changes and hopefully we can land both.
On Tuesday, February 14, 2017 at 6:47:21 AM UTC-8, mpl wrote: > > > > On 14 February 2017 at 04:41, Steve Armstrong <[email protected] > <javascript:>> 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/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]. >>>> 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.
