Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update docs and examples to reflect that BridgeConfig.apiPath must end with / #3654

Open
spollard opened this issue Aug 1, 2022 · 4 comments
Labels
app-platform App/Sandstorm integration features bug

Comments

@spollard
Copy link
Contributor

spollard commented Aug 1, 2022

I was creating a new python app using the diy stack and wanted to allow an HTTP API, but simply uncommenting the apiPath = "/api" in sandstorm-pkgdef.capnp didn't work for sub paths such as
http://api-8a22a421a78b1ecbdf92b9a691b7ffda.local.sandstorm.io:6090/stuff

The grain log shows the resulting url to be /apistuff rather than /api/stuff:
127.0.0.1 - - [01/Aug/2022 01:48:18] "GET /apistuff HTTP/1.1" 202 9
and this is because according to sandstorm.io/sandstorm/src/sandstorm/package.capnp:218 the BridgeConfig.apiPath must end with "/".

So the documentation and many examples give "/api" as the example apiPath value, but it doesn't fit the requirements for that value.
I searched across sandstorm repos for apiPath examples and it looks like many of them do not end with a /. See https://github.com/search?q=org%3Asandstorm-io+apipath&type=code .

Should all those examples and documentation be updated from "/api" to "/api/"? Are there any downsides to ending with a /? If your api doesn't use subpaths, then there may be no substantial difference between /api and /api/, except that the docs say it needs to end with /.

I believe updating the docs across the repos would be easy. Just use this search https://github.com/search?q=org%3Asandstorm-io+apipath&type=code and change all the examples that are wrong.

@ocdtrekkie
Copy link
Collaborator

Unfortunately, I think the output of spk init at present gives people #apiPath = "/api", in their default pkgdef, so we need to correct it in the code as well. I noticed apps I don't use this have that, and the one I just built which does use it, I had to add the trailing /.

@ocdtrekkie ocdtrekkie added bug app-platform App/Sandstorm integration features labels Aug 1, 2022
@zenhack
Copy link
Collaborator

zenhack commented Aug 1, 2022

Note that the relevant spot in the spk source shows up in the search results :P

The only drawback I see is that I'm not sure /api (with no trailing slash) will actually be accessible if you leave the trailing slash on. But maybe that's ok.

@ocdtrekkie
Copy link
Collaborator

Well, the comment on line 218 suggests that isn't an option. If the trailing slash is required, and you needed say... just a file called API, you'd need the apiPath to just be /, right?

@zenhack
Copy link
Collaborator

zenhack commented Aug 1, 2022

Well, there you go. Then yeah, we should just change this everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app-platform App/Sandstorm integration features bug
Projects
None yet
Development

No branches or pull requests

3 participants