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

Faro receiver component #32

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Faro receiver component #32

merged 5 commits into from
Feb 4, 2025

Conversation

t00mas
Copy link
Contributor

@t00mas t00mas commented Jan 29, 2025

No description provided.

@t00mas t00mas requested a review from rlankfo January 29, 2025 15:56
Copy link
Member

@rlankfo rlankfo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress! Left a comment around cors handling, otherwise this looks like it is coming together.

Comment on lines 80 to 82
resp.Header().Set("Access-Control-Allow-Origin", "*")
resp.Header().Set("Access-Control-Allow-Methods", "POST, OPTIONS")
resp.Header().Set("Access-Control-Allow-Headers", "Content-Type")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The confighttp.ServerConfig struct we embed has a CORS member we can configure. We shouldn't have to handle cors ourselves but we might want to add * in our default config because if left unset, cors will be disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's true - I'll fix this right away.

@t00mas t00mas marked this pull request as ready for review January 31, 2025 16:46
@t00mas t00mas requested review from rlankfo and mar4uk January 31, 2025 16:46
)

const (
pbContentType = "application/x-protobuf"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this variable used anywhere? couldn't find

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think it's leftover from the draft, will remove.

@t00mas t00mas merged commit 095d058 into main Feb 4, 2025
2 checks passed
@t00mas t00mas deleted the faro-receiver branch February 4, 2025 15:58
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.

3 participants