usdot-fhwa-stol / carma-streets

CARMA Streets is a component of CARMA ecosystem, which enables such a coordination among different transportation users. This component provides an interface for CDA participants to interact with the road infrastructure. Doxygen Source Code Documentation: https://usdot-fhwa-stol.github.io/documentation/carma-streets/
10 stars 10 forks source link

Streets Service Base Lanelet Aware Docker Image #361

Closed paulbourelly999 closed 1 year ago

paulbourelly999 commented 1 year ago

PR Details

Description

Create base image for CARMA Streets services like Message Service, Intersection Model, and future Sensor Data Sharing Service that includes lanelet2 dependency.

Related GitHub Issue

Related Jira Key

CDAR-389

Motivation and Context

Create base image for Sensor Data Sharing Service and any other carma-streets services that require lanelet2

How Has This Been Tested?

Locally built base image

Types of changes

Checklist:

paulbourelly999 commented 1 year ago

General comment/question: Why do we have multiple Dockerfiles for streets_service_base related images? They should be merged into a single, mutli-stage Dockerfile unless there is a compelling reason not to.

My thought process is there are two different base images created here. Most streets services will only need streets_service_base but some services need an understanding of the geometry of the intersection. This is where the lanelet aware base image comes into play. Just to illustrate this I can outline what current carma-streets services should have which base image.

Scheduling Service : FROM streets_service_base Signal Optimization Service: FROM streets_service_base TSC Service: FROM streets_service_base Intersection Model: FROM streets_service_base_lanelet_aware Message Service: FROM streets_service_base_lanelet_aware Sensor Data Sharing Service: FROM streets_service_base_lanelet_aware

I could do this with a single docker file and a multi-staged build, but it may be more readable to have 2 separate docker files. The only other thing I am concerned about is if I can get docker hub automated builds to push intermediate stages as images as well. If I can get that to work I will combine the two into a single docker file

adamlm commented 1 year ago

I could do this with a single docker file and a multi-staged build, but it may be more readable to have 2 separate docker files. The only other thing I am concerned about is if I can get docker hub automated builds to push intermediate stages as images as well. If I can get that to work I will combine the two into a single docker file

@paulbourelly999 Are you recommend we push that to a future issue, or do you want me to wait on the review until you get that working?

paulbourelly999 commented 1 year ago

mmend we push that to a future issue, or do you want me to wait on the review until you get that working?

I looked into it, dockerhub automated builds and it does not offer any way that I have found to specific --target in a multi-stage build, which means that a multi-stage dockerfile in this case would only be able to push the final stage image. Alternatively we could use github actions to build the base images instead, which I am ok with. I am still in favor of keeping these as separate Dockerfiles since it makes the architecture easier to handle. Multi-stage build does not allow you to specify the tag of a build stage so I can no longer make streets_service_base_lanelet_aware use streets_service_base:bionic. I want to be able to push streets_service_base for bionic, focal and jammy and then build off those. I think for that reason it makes more sense to keep these separate.

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.11) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. Read more here