-
Notifications
You must be signed in to change notification settings - Fork 5
Wip development #154
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
base: master
Are you sure you want to change the base?
Wip development #154
Conversation
|
HI Thank you for the PR. |
| import.meta.env.MMP_BACKEND_PATH | ||
| ].join(''); | ||
|
|
||
| const BACKEND_HTTP_URL_ROOT = `http://${BACKEND_SCHEMALESS_URL_ROOT}`; |
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.
Hard-coding http might cause issues if the user is using a secured reverse proxy
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.
You're right. I'll move that so it is a part of the environmental variable. This is an artifact of when I thought the SSE were websockets and had a second variable exported.
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.
Btw I agree with @alyxdeburca, let's keep the current env file as a fallback behavior to reduce breaking changes.
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.
OK. I can readd it.
|
I haven’t had time to review the code but might I suggest allowing settings to be loaded from the json file as a failsafe if the environment variables cannot be loaded, just to keep the breaking changes to a minimum |
|
@alyxdeburca Funny enough, that is why I switched to environmental variables. If the EDIT: I'll readd it to prevent breaking changes. |
|
@Thrilleratplay, please correct me if I'm wrong, but the values from the .env are baked into the source code at transpile time, right? |
|
@EduardoOliveira I think the confusion is due to using the wrong port in the To answer your question, the default values are baked in. If we use the docker-compose.yml, this is what currently exists: environment:
- "AGENT_ADDRESS=agent:8000" The changes I made would make this unnecessary and unused as the agent is exposed on After I correct this in the PR, the defaults will be: environment:
MMP_BACKEND_HOST="http://localhost:8000"
MMP_BACKEND_PATH="/api"So if a user wanted to have the UI and agent on different machines, or through a reverse proxy, they could adjust these as necessary. Hopefully that clarifies things. I will make sure it will work with the default values and readd the |
This is a very much a work in progress and is largely untested at this point. I am approaching this in three phases.
Thus far:
TO DO: