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

MiniLangStream enable NodePort if missing for services #701

Merged
merged 7 commits into from
Nov 8, 2023

Conversation

Gagravarr
Copy link
Contributor

MiniLangStream should enable NodePort as required

If start_port_forward finds that there's currently no Target Port and hence no external URL for a service, it should patch the service definition to enable a dynamically allocated NodePort.

This fixes #698

Nick Burch added 3 commits November 7, 2023 19:50
Allow specifying the LangStream home directory, via the
LANGSTREAM_HOME property (or environment variable).

If not specified, continue to use .langstream in the user's
home directory, but allow that to be a symlink. Fixes LangStream#697
Add error handling to the shell script function `start_port_forward`
to avoid the current hanging if no *node port* is defined for the
service.

Doesn't attempt any alternate forwarding (eg kubectl), just gives
a moderately helpful message and errors out.
If start_port_forward finds that there's currently no Target Port
and hence no external URL for a service, it should patch the
service definition to enable a dynamically allocated NodePort.
Copy link
Member

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

Bash functions return all the stdout, so have the kubectl patch
output from enabling the NodePort discarded, so that only the
URL is returned.
@eolivelli
Copy link
Member

@Gagravarr can you please solve the conflicts ?

Nick Burch added 3 commits November 8, 2023 14:04
Also means that if Minikube kicks out an error and suggests a link
to their site after reporting the service details, you don't
additionally get their github URL coming through!
@Gagravarr
Copy link
Contributor Author

Conflict solved (sorry, took this branch of the wrong one so clashed with other changes)

Also one small fix added - if there are multiple URLs found when querying for a service, only the first is taken

@eolivelli eolivelli merged commit 1d32f33 into LangStream:main Nov 8, 2023
10 checks passed
@eolivelli
Copy link
Member

Merged! thanks

benfrank241 pushed a commit to vectorize-io/langstream that referenced this pull request May 2, 2024
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.

mini-langstream start hangs after HerdDB - service langstream/langstream-control-plane has no node port
3 participants