-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix: add routes data for panel api endpoints #263
Conversation
@sea-grass, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction. |
@sea-grass, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed. |
@sea-grass, VMware has approved your signed contributor license agreement. |
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.
Thank you @sea-grass. I can confirm the issue, and that this PR fixes the problem.
I also think it is a great way to approach this problem. The circular dependency problem could be solved by using a different crate and make that one being depended upon by the other two, but I think this is good enough!
Hey @sea-grass ! Thank you for the fix here. In the future we plan to add dynamic routing, so this may require a refactor later on but for now is a great solution. I already triggered the CI and will merge after it! Thank you for your contribution 😄 |
@sea-grass there is an issue that comes from the |
@sea-grass I can confirm the |
Btw @sea-grass, I want to take the opportunity to ask you about the admin panel. It is an experimental feature and we have ideas to expand it in the future. What would you expect from this admin panel? Any specific data / action you would like to have there? Thank you! |
Hmm that's a good question! I'm not using WWS in any production capacity yet so I'll be speculating a bit, but when I think admin panel I think metrics related to application performance/observability and some configurability regarding behaviour. Something like:
|
Thank you for taking the time to share this feedback! This is really valuable as we already discussed in the past some of the features you mentioned, like response metrics. Regarding enable/disable workers, it's an interesting feature when working on an environment with many workers or shares the same instance between different projects 😄 |
When visiting the admin panel after running
wws --enable-panel
, we are presented with a loading spinner and the worker info never loads. WithRUST_LOG="actix_web=debug"
, we can see the following error when we access the panel athttps://127.0.0.1:8080/_panel
:This PR adds Routes through the app's app_data for the api handler. I don't typically work in Rust, so let me know if there's a better/more preferred approach to solve this problem!
Note: The routes are available as part of the
AppData
that's already exposed to the Actix app, but addingwws-server
as a dependency towws-api-manage
so we can reference theAppData
type results in a circular dependency.