-
Notifications
You must be signed in to change notification settings - Fork 8
Addressing small bugs with fixes #15
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: main
Are you sure you want to change the base?
Conversation
Shreyanand
left a comment
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.
Thanks for the PR! added some discussion points.
| from autogen_agentchat.base import TaskResult | ||
|
|
||
| # Import predefined models and manager | ||
| from backend.core.agents import AgentManager, IssueRequest, IssueResponse |
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.
Wouldn't this give an error on the Openshift pod, since it would run from the root of the repository?
For a local run, would running the server from root of the repository instead of the backend directory solve this issue?
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.
Yeah if we update the instructions in the README to run from root instead of backend dir then we don't need to do this
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.
It looks good overall, If we revert this change and update the README, I'll go ahead and merge this in.
Thanks for the fixes!
|
|
||
| mkdir -p /opt/app-root/src/config | ||
|
|
||
| cat <<EOF > /opt/app-root/src/config/config.js |
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.
that's a clear bug, I don't know how it is working now :)
The point of adding this config file was to make the backend API URL configurable. The react app generates pages and settings during build time and so it was not straightforward to have a runtime environment variable for the backend API. I'm open to suggestions / alternatives to this approach, my understanding here is limited.
We'd have to test any changes by rebuilding the image and testing the deployment on OpenShift. We need Github Actions.
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.
so the bug I got was basically config.js file not found and since the actual file name is config.ts once I updated it, it worked...not sure how it worked on OpenShift if the filename itself doesn't exist?
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.
makes sense!
When running the setup locally I ran into few bugs for which I have fixes in this PR