Skip to content
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

fix: camera mgmt docker build, secret names, and go mods #229

Merged

Conversation

ajcasagrande
Copy link

  • Fixed the docker build for Camera Management App and make it work with docker compose.
  • Fixed the naming inconsistencies with secrets in the yaml file and code. Now also matches rtspauth from the device-usb-camera service.
  • Removed errors package from go.mod, to use EdgeX errors package, to match the device services.

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-examples/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • [N/A] I have added unit tests for the new feature or bug fix (if not, why?)
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?)

Testing Instructions

- Fixed the docker build for Camera Management App and make it work with
  docker compose.
- Fixed the naming inconsistencies with secrets in the yaml file and
  code. Now also matches rtspauth from the device-usb-camera service.
- Removed errors package from go.mod, to use EdgeX errors package, to
  match the device services.

Signed-off-by: Anthony Casagrande <[email protected]>
@vyshali-chitikeshi
Copy link
Contributor

Valediction looks good in non-secure mode. Tested with ONVIF and USB cameras.
Default pipelines starts automatially, manual start/stop of pipeline works, streaming of events works for both ONVIF and USB cameras. On ONVIF also verified PTZ operations. It works correctly

@@ -14,17 +14,39 @@
# limitations under the License.
#

version: "3"
version: '3.7'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not that important but the newest version is 3.8 right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I looked into this more and seems like you kept this in sync with other edgex compose files.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important.

Version 3.8
An upgrade of version 3 that introduces new parameters. It is only available with Docker Engine version 19.03.0 and higher.

Introduces the following additional parameters:

max_replicas_per_node in placement configurations
template_driver option for config and secret configurations. This option is only supported when deploying swarm services using docker stack deploy.
driver and driver_opts option for secret configurations. This option is only supported when deploying swarm services using docker stack deploy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version field is actually deprecated and merely informative now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know!

}
}
if err != nil {
app.lc.Errorf("Unable to query EVAM pipeline statuses after %d tries. .")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to put in the value of i for this error message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there is an extra period in the message.

Copy link
Contributor

@ChristianDarr-personal ChristianDarr-personal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Anthony Casagrande <[email protected]>
Copy link
Contributor

@ChristianDarr-personal ChristianDarr-personal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

# Run the app.
make run-app
# Start the docker compose services in the background for both EVAM and Camera Management App
docker compose up -d
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all other commands are make commands it would be nice to also have docker compose ones under make.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally I would agree, but in this case I think it is fine to keep as docker compose, as the user has more control over it

Copy link
Contributor

@presatish presatish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have made a nice to have comment though.

@ajcasagrande ajcasagrande merged commit 04d6a19 into edgexfoundry:main Jun 5, 2023
@ajcasagrande ajcasagrande deleted the docker-compose branch June 5, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants