search

citros-garden / robotic_arm

Robotic arm example for CITROS integration
0 stars 0 forks source link

Citros ready #2

Closed gtep96 closed 11 months ago

gtep96 commented 11 months ago

Project ready and running in CITROS

gtep96 commented 11 months ago

hi @gtep96,

A few points regarding the Dockerfile:

  • [ ] please remove WORKDIR command duplications.
  • [ ] please try to group commands by logic, meaning try to do only one apt update + apt install, one pip install,
  • [ ] because we use custom docker image, I think the user ros already defined, so no need to define it again in this dockerfile - I think I did it few month ago, but it worth fix now.
  • [ ] no need cyclonedds you can remove it (but check that it works)

Thanks.

Hi @iftahnaf,

I've updated this branch with these fixes. Although, i didn't find 'ros' user definition in the dockefiles, only 'dev' user. Dont you think it's necessary?

iftahnaf commented 11 months ago

hi @gtep96, A few points regarding the Dockerfile:

  • [ ] please remove WORKDIR command duplications.
  • [ ] please try to group commands by logic, meaning try to do only one apt update + apt install, one pip install,
  • [ ] because we use custom docker image, I think the user ros already defined, so no need to define it again in this dockerfile - I think I did it few month ago, but it worth fix now.
  • [ ] no need cyclonedds you can remove it (but check that it works)

Thanks.

Hi @iftahnaf,

I've updated this branch with these fixes. Although, i didn't find 'ros' user definition in the dockefiles, only 'dev' user. Dont you think it's necessary?

Hi @gtep96,

I think we shouldn't create the dev user, because, in the base image, there is already ros user, so we just need to replace everything related with dev user to ros, and remove the dev user creation.

gtep96 commented 11 months ago

Done, tested, pushed

hi @gtep96, A few points regarding the Dockerfile:

  • [ ] please remove WORKDIR command duplications.
  • [ ] please try to group commands by logic, meaning try to do only one apt update + apt install, one pip install,
  • [ ] because we use custom docker image, I think the user ros already defined, so no need to define it again in this dockerfile - I think I did it few month ago, but it worth fix now.
  • [ ] no need cyclonedds you can remove it (but check that it works)

Thanks.

Hi @iftahnaf, I've updated this branch with these fixes. Although, i didn't find 'ros' user definition in the dockefiles, only 'dev' user. Dont you think it's necessary?

Hi @gtep96,

I think we shouldn't create the dev user, because, in the base image, there is already ros user, so we just need to replace everything related with dev user to ros, and remove the dev user creation.