-
Notifications
You must be signed in to change notification settings - Fork 0
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
added env vars to docker #17
Conversation
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.
what part of this system setup will run without docker?
I can run through this today during software meeting. |
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 would like to improve the readability of the .bashrc
file modification script, but this could definitely be done in a separate PR. Otherwise, looks good. Your call on whether to make these changes now.
echo 'cd rb_ws' >> ~/.bashrc && \ | ||
echo 'colcon build --symlink-install' >> ~/.bashrc && \ | ||
echo 'source install/local_setup.bash' >> ~/.bashrc && \ | ||
echo 'chmod -R +x src/buggy/scripts/' >> ~/.bashrc | ||
echo 'chmod -R +x src/buggy/scripts/' >> ~/.bashrc && \ | ||
echo 'source environments/docker_env.bash' >> ~/.bashrc |
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.
This could be a separate refactor, but I think making this a single command across several lines would be more readable.
This would look something like
echo -e '\
source "/opt/ros/humble/setup.bash" -- \n\
cd rb_ws \n\
colcon build --symlink-install \n\
...
' >> ~/.bashrc
The key to this is the -e
flag passed to echo
that enables the \n
newline character, and using \
to include multiple lines in the command. Writing it this way makes the readability of the actual commands we want to run more apparent.
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.
the yelling worked, approved
What type of PR is this? (check all applicable)
Description
Describe the CHANGES, the REASONING, and BENEFITS of this PR.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?