Conversation
IoanAndrasi
left a comment
IoanAndrasi
left a comment
There was a problem hiding this comment.
Great work for initial work, what I would also mention is that some files are very well documented with comments while others lack's comments.
| id: claude-review | ||
| uses: anthropics/claude-code-action@v1 | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} |
There was a problem hiding this comment.
Who is providing this secret key ?
If token usage is hit maybe we need another secret key as backup ?
There was a problem hiding this comment.
We will get copilot support, not sure when.
| } | ||
|
|
||
| struct TopologyInner { | ||
| state: RwLock<TopologyState>, |
There was a problem hiding this comment.
Shouldn't we use tokio::sync::RwLock for all locks which is more ideal for async context ?
std::sync::RwLock blocks the threads which increase the risk of task starvation.
There was a problem hiding this comment.
Thanks for your remark. As long as we do not hold the lock across async points we are safe, rust type system will catch this anyhow. The code is correct.
But you triggered some thoughts whether we can get rid of Arc since we hold a RW lock. Let me come back to you once the idea is settled to show you an alternative.
| Router::new() | ||
| .route("/components", get(component_list)) | ||
| .route("/components/{component_id}", get(component_capabilities)) | ||
| .route("/components/{component_id}/hosts", get(component_hosts)) |
There was a problem hiding this comment.
Shouldn't we follow the standard and use /components/{components_id}/related-apps ?
There was a problem hiding this comment.
@IoanAndrasi looking at the v1.1 ISO related-apps is deprecated. If you need it for backward compatibility we can add it.
|
Changed merge target to inc/liebherr |
lh-sag
force-pushed
the
inc/liebherr
branch
2 times, most recently
from
0f4f3c3 to
362947d
Compare
|
One thing I noticed is that the repo doesn’t include a config.toml, which means the Rust build settings aren’t configured yet. |
|
I have also tested docker container on personal pc and work laptop and on both I encountered the same issue, when I run for the first time dev container it fails to attach to image, after I remove, "overrideCommand": false from devcontainer.json, and try to rebuild the docker image then dev container works. |
|
I ran python integration tests as mentioned in docs/testsing.md with uv run pytest and 16 tests fail. |
|
According to IANA, Sovd is recommended to run on port 7690. |
This is fixed. I upgraded to python 3.14 which lacks certain pre-build packages and during compilation from sources it run into OOM due to constrains in the devcontainer. Switched to python 3.13 and this works now.
This is a cascading failure as the crypt module was not build. Should be fixed by now. |
The CI builds for all platforms and architectures. If you want to (cross) build locally I recommend https://github.com/cross-rs/cross Do build for aarch64 Give it a try and let me know whether it works for you. |
2 participants
Summary
Incubator for OpenSOVD Core
Checklist
Related
Notes for Reviewers