Feat/subject tracking#219
Conversation
jrobble
left a comment
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @brosenberg42)
docker-compose.components.yml line 47 at r1 (raw file):
environment: <<: *common-env-vars WFM_USER:
For consistency, let's create a wishlist task to go back and update the detection components to perform registration like the subject tracking components.
There are a number of other improvements we can backport to the detection components and classes as well, like DB access. Please add them all to the same wishlist task. Anything you can think of.
subject-components/python/Dockerfile line 105 at r1 (raw file):
ENV COMPONENT_LANGUAGE python # Using tini here is equivalent to starting the container with the --init flag, except that users
What's your motivation for using tini?
Should we use this for all detection and subject components?
brosenberg42
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jrobble)
subject-components/python/Dockerfile line 105 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
What's your motivation for using tini?
Should we use this for all detection and subject components?
There are other benefits, but the primary reason is to ensure signals are handled properly.
It isn't necessary for the detection components because component-executor.py has signal handling code. We would need to remove or update that code if we want to use tini. If we change detection component to register through ActiveMQ, we might need to use tini.
jrobble
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)
docker-compose.components.yml line 47 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
For consistency, let's create a wishlist task to go back and update the detection components to perform registration like the subject tracking components.
There are a number of other improvements we can backport to the detection components and classes as well, like DB access. Please add them all to the same wishlist task. Anything you can think of.
Please drop a link to the issue here.
brosenberg42
left a comment
There was a problem hiding this comment.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @jrobble)
docker-compose.components.yml line 47 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Please drop a link to the issue here.
jrobble
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @brosenberg42)
jrobble
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)
jrobble
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)
openmpf_build/Dockerfile line 106 at r4 (raw file):
FROM apt_install
https://openmpf.github.io/docs/site/Development-Environment-Guide/index.html#setup-vm needs to be updated with these new setup steps so devs can build on their VM.
brosenberg42
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jrobble)
openmpf_build/Dockerfile line 106 at r4 (raw file):
Previously, jrobble (Jeff Robble) wrote…
https://openmpf.github.io/docs/site/Development-Environment-Guide/index.html#setup-vm needs to be updated with these new setup steps so devs can build on their VM.
I added a PR for that: openmpf/openmpf.github.io#191
jrobble
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)
Issues:
Related PRs:
This change is