-
Notifications
You must be signed in to change notification settings - Fork 50
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
Adding racs2 /cfs-ros2 bridge demo by JAXA on spaceros #81
base: main
Are you sure you want to change the base?
Conversation
I just added myself as reviewer for this PR. I'd like to work with @yuyuqq on 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.
I started reviewing however I got blocked with confusion: Is having the docker
directory necessary here? It looks like a copy of the demos from the docker repo.
To reduce duplicate code, my first thought is that the docker repo should be a submodule, however I noticed that space_robots clones this repository via vcs within it's Docker workflow, so this is looking circular.
My second thought is that the racs2 demo docker image should be built from the docker repository and any lower level packages/nodes/cfs applications should be here in the demo repository, however this is conflicting with the demo README.
Overall I could use some clarity on the purposes of the docker and demo repositories @ivanperez-keera.
RUN python3 -m pip install protobuf==3.20.0 websockets==12.0 | ||
|
||
# Get the cFS source code | ||
RUN git clone --recursive -b v6.7.0a https://github.com/nasa/cFS/ cfs |
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.
I'm not a fan of all of this git clone
ing in the Dockerfile, I've learned that this can cause issues since Docker doesn't know when to invalidate the layer cache when the upstream changes.
It would be nice if these repositories were a git submodule, downloaded via vcs before the Dockerfile is run, or at the very least, uses the ADD
Dockerfile instruction .
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.
I think this kind of clone is fine in this case because we are cloning a specific tag. The code won't change unless the tag changes, and that shouldn't happen because it's a release tag.
If it was a rolling HEAD, then I agree that we might want to do it differently.
racs2_demos_on_spaceros/Dockerfile
Outdated
|
||
# Get the RACS2 source code | ||
WORKDIR ${RACS2_DEMO_DIR} | ||
RUN git clone https://github.com/jaxa/racs2_bridge |
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.
Can this repo be pinned to a specific branch/version?
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.
I have a few suggestions for this Dockerfile, but generally I'm conflicted about how this is organized.
I appreciate that this goes step by step and documents how to use the RACS2 bridge, but there is a lot more file manipulation here than I would typically expect from a Dockerfile.
Since this is a demo I suppose this doesn't need to be productionized and is better off as it is now as documentation?
I think I'd feel better about having all of the code existing in the repository with this file manipulation done before-hand if this process was also documented elsewhere and referenced in this codebase (e.g. in the README or as a comment here). However, I understand that could be a lot more additional work, so I'm happy either way.
racs2_demos_on_spaceros/README.md
Outdated
``` | ||
You will need three terminals to run demo. | ||
|
||
For instractions after entering docker, please read docker/racs2_demos_on_spaceros/README.md |
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.
For instractions after entering docker, please read docker/racs2_demos_on_spaceros/README.md | |
For instructions after entering docker, please read docker/racs2_demos_on_spaceros/README.md |
We met today with the JAXA team to discuss this pull request and walk them through some of the changes. They'll be working on that and submitting an updated version to this same PR. |
4ff2dda
to
2893e67
Compare
2893e67
to
3458c2a
Compare
Title: Adding racs2 /cfs-ros2 bridge demo by JAXA on spaceros
Description:
This pull request adds the racs2_demos_on_spaceros demo to the SpaceROS project. (racs2 /cfs-ros2 bridge demo by jaxa)
(Issue #26 )
Changes include:
Purpose:
This addition aims to demonstrate a function and usage of RACS2 bridge in the SpaceROS project, providing a tool for system design and showcasing functionalities.
Testing:
All new features were tested in the provided simulation environment to ensure proper functionality and integration. No issues were found.
Impact:
These changes are backward-compatible and do not affect existing functionalities.
Additional Information:
For more details, please refer to the included README file or the project documentation.