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

chore(eigenda): Bump eigenda-proxy to latest v1.6.0 #48

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

epociask
Copy link
Collaborator

@epociask epociask commented Dec 3, 2024

Changes

  • Updated proxy dependency to v1.6.0
  • Updated simple client logic to be compatibility with new put route
  • Fixed go fmt issue with service unavailable error

Comment on lines +23 to +26
if [ $? -ne 0 ]; then
echo "==== Failed to start eigenda-proxy container ===="
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this will catch if docker didn't manager to start the process, but won't catch if the process is started but crashed on startup.

Comment on lines -130 to -131
if resp.StatusCode == http.StatusServiceUnavailable {
return nil, SvcUnavailableErr
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you really mean to remove this? Is ErrSvcUnavailable returned somewhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahh incredible catch 🙏🏻

return nil, fmt.Errorf("received unexpected response code: %d", resp.StatusCode)
b, err := io.ReadAll(resp.Body)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrap err with resp.StatusCode

Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM. Just to be sure you deleted the ErrServiceUnavailable error returned in the GET path. That's the intended behavior right? To only have that in POST path?

@epociask epociask merged commit 238699d into eigenda-v3.2.1 Dec 3, 2024
8 checks passed
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