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

Include response headers in real sat response #43

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

SteveSandersonMS
Copy link
Contributor

Hi there! This project looks extremely promising.

I was trying out sat and got quite confused about why calls to resp::set_header didn't seem to have any effect - my HTTP headers were not showing up on the response. On looking into it, I see they do get added to the CoordinatedResponse structure, but the RespHeaders here are simply ignored by handler.go and hence they don't end up in the real HTTP response.

Fortunately it's very easy to add the headers to the real HTTP response as per this PR. If there is some reason why it's intentional that the response headers should be discarded then I apologise (though it would be great if you could clarify why). Thanks!

@cohix
Copy link
Contributor

cohix commented Mar 14, 2022

Amazing! Thanks for doing this, and sorry for the delay in reviewing :)

@cohix cohix merged commit d884952 into suborbital:main Mar 14, 2022
@SteveSandersonMS
Copy link
Contributor Author

No worries - thanks for merging!

I'm curious about your future plans for Sat too. A few things make it difficult to use it to serve arbitrary web endpoints:

  • Can only accept POST requests (not GET or other methods)
  • Can only send custom status codes in the response if treating it as an error response
  • The /meta/* endpoints are public

Is it your intention for Sat to be usable for arbitrary web APIs/servers, or is it intentionally restricted because of specific goals about what it should be used for?

@cohix
Copy link
Contributor

cohix commented Mar 14, 2022

@SteveSandersonMS Our pleasure!

The plans are for this to be as flexible as it can be within the mandate of "be small, fast, and simple".

If you'd like to file issues for the things mentioned (and any other feedback you have), I'd be happy to chat about them each. Everything you listed is certainly reasonable and would likely be in scope, they just haven't been needed so far!

Glad you've put Sat through its paces, and thank you very much for taking the time to ask about improvements 🙂

@SteveSandersonMS
Copy link
Contributor Author

Thanks for the info. As requested, I've filed #47 and #48 to represent these suggestions.

@cohix
Copy link
Contributor

cohix commented Mar 14, 2022

Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants