-
Notifications
You must be signed in to change notification settings - Fork 45
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
Develop Update Submodules #515
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.
@TonyEnglish @Michael7371 Discovered issue with this fork. Was able to build docker images successfully by using git clone --recurse-submodules but ran into issue running kafka container from docker compose. Issue is related to https://github.com/Michael7371/jpo-ode/blob/47d21847a2f087545d7e9ff367e70b07478c881a/docker-compose.yml#L37
https://github.com/Michael7371/jpo-ode/blob/47d21847a2f087545d7e9ff367e70b07478c881a/sample.env#L33.
Kafka container fails to start with the following docker compose error :
Error response from daemon: invalid volume specification: 'C:/var/run/docker.sock:/var/run/docker.sock:rw'
After removing ${DOCKER_SHARED_VOLUME_WINDOWS} from volume declaration in docker compose the issue goes away. There is no README instruction to edit this environment variable at all in installation process so maybe the README of the sample.env needs to be updated. What do you guys think? If this is not concerning I can approve the PR.
NOTE: Only tested docker build and docker compose up. Please let me know if you want me to test specific functionality
Hello @paulbourelly999, nice find! I was struggling to reproduce this error until I tried to use “docker compose up -d” from my WSL Ubuntu shell directly and that seemed to produce the error. Windows PowerShell didn't seem to have an issue creating the volume with the “DOCKER_SHARED_VOLUME_WINDOWS” variable set to either the “C:” value or an empty value. Let me know if this checks out with what you are seeing. I think for now I will change the default value for that variable in the sample.env to be empty and add some documentation to the README about this error if the behavior above is the same for you. |
@paulbourelly999 I pushed those changes an also created an issue to address this fully in a seperate PR: #517 |
That sounds perfect. For some reason the changes are not showing up here but are in the other PR. Is this an issue.? |
… set the default value to empty
Yes, good check! I just pushed a commit to this branch with those changes. |
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.
Looks good
PR Details
Description
Updating submodules using this command: git submodule update --remote --merge
Related Issue
N/A
Motivation and Context
Includes many changes, one of which is that this commit includes the J2735 ASN file. Also noticed that many other modules were out of date.
How Has This Been Tested?
The submodules updated have already been thoroughly tested, this just updates the reference to those modules.
Types of changes
Checklist:
ODE Contributing Guide