-
Notifications
You must be signed in to change notification settings - Fork 2
[Configs] Add asset configuration #201
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?
Conversation
|
🧱 Stack PR · Base of stack (2 PRs total) Stack Structure:
|
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.
Code Review
This pull request successfully adds asset configuration capabilities to the simulation, which is a valuable enhancement. The changes are consistently applied across protobuf definitions, generated code, and configuration files. The associated test updates are also appropriate. My main feedback is a suggestion to address the duplication of the new asset_configs block across multiple simulation files to improve long-term maintainability. Additionally, as a general process improvement, consider separating dependency updates from feature work into separate pull requests. This can make reviews more focused and simplify rollbacks if any issues arise.
📝 WalkthroughWalkthroughThis PR adds an asset-level configuration to simulations by importing agent_config.proto and adding Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/MODULE.bazel (1)
3-30:⚠️ Potential issue | 🔴 CriticalAdd protobuf 33.5 override; it is not yet published in Bazel Central Registry.
The protobuf version 33.5 does not exist in BCR (which currently has up to 33.4 only), so
bazel_dep(name = "protobuf", version = "33.5")will fail to resolve. Use anarchive_overrideorgit_overrideto point to the upstream protobuf v33.5 release until BCR publishes it. Version consistency with Assets/Proto/MODULE.bazel is correct; other dependencies (rules_cc 0.2.16, abseil-cpp 20260107.0, googletest 1.17.0) are compatible with Bazel 8.5.1.
This PR adds an additional field to the simulation configuration for asset configurations. Since I have to regenerate all plugins (as the proto files changed), I figured I might as well bump all package versions.