-
Notifications
You must be signed in to change notification settings - Fork 197
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
Improve local mode #361
Improve local mode #361
Conversation
|
@@ -37,6 +42,7 @@ ENABLE_GO_PARSER="false" \ | |||
GO_PARSER_API_URL="http://go-parser:7777" \ | |||
ENABLE_TOKEN_MATCHING_CHECK="false" \ | |||
vector \ | |||
-qq \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: imo it would be more readable to use VECTOR_LOG
try { | ||
const url = new URL(_jsonData.apiServerUrl); | ||
if (url != null) { | ||
apiConfigs.prefixUrl = url.toString().replace(/\/$/, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see. So the hack is we reset this apiConfigs
global var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well the real hack is that we force everything to read the config on every request as opposed to having it pre-baked in the ky server instance hah
@@ -699,20 +719,20 @@ const api = { | |||
}, | |||
useInstallation() { | |||
return useQuery<any, HTTPError>(`installation`, () => | |||
server(`installation`).json(), | |||
hdxServer(`installation`).json(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the default method is GET
@@ -20,7 +20,7 @@ export default function PasswordResetPage({ | |||
<div> | |||
<Form | |||
className="text-start" | |||
action={`${API_SERVER_URL}/password-reset`} | |||
action={`${SERVER_URL}/password-reset`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double check that this url won't be updated dynamically, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is true, though this isn't important in local mode...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll likely want to wrap this in a context and pull it from there in the future, but that's out of scope for this PR for now.
Makes a number of improvements to local mode:
General changes:
NEXT_PUBLIC_SERVER_URL
typo in docker-compose (and standardize constant name exported from config)