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

Add build and containerize pipeline #61

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

ValMobBIllich
Copy link
Contributor

@ValMobBIllich ValMobBIllich commented Jan 13, 2025

Adds a new action (and a small composite action) that builds the streamer binary, creates an image with it and pushes it to a container registry. Users can then pull the image from the registry and run the streamer in a container without ever looking at the source code.

The pipeline build statically linked binaries of the zenoh-mqtt streamer and pushes them via a manifest so that users can pull whichever image is better for their platform.

There is also a new docker directory which showcases how to use the images in the container registry and with a complete example config setup. Maybe there is a way to structure this thats a bit easier to understand? For now I ll just add some documentation in the implementations Readme...

@PLeVasseur PLeVasseur self-requested a review January 14, 2025 14:43
@PLeVasseur
Copy link
Contributor

I'm guessing I should hold on review till the CI passes & you give me a heads up. My assumption though! Let me know.

@ValMobBIllich
Copy link
Contributor Author

I'm guessing I should hold on review till the CI passes & you give me a heads up. My assumption though! Let me know.

Yeah I think we need to merge the other PR first so that the vsomeip stuff works again? The build and push to registry pipeline fails right now because Im using the organization name for the image URL and that has to be lowercase (our is ValtechMobility so its not...) but eclipse-uprotocol should be fine. Anyways the build pipeline wont run on PR but on push to main so it shouldnt block the PR once I change that.

@PLeVasseur
Copy link
Contributor

Hey @ValMobBIllich -- looked like CI fell over here. Perhaps need to rebase? I'd like to get CI running and then take a look :)

@PLeVasseur
Copy link
Contributor

Hey @ValMobBIllich! I'm going to start reviewing this now. Apologies if I pick over things you did a while back, I'm looking at it with fresh eyes. 🙏

Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Hey @ValMobBIllich! Thanks for the contribution to containerize. Could you take a look at some of the suggestions I left?

# SPDX-License-Identifier: Apache-2.0
# *******************************************************************************/

name: "Setup Rust and Cross"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use cross-rs? I have the stable Rust compiler installed and a

rustc --print target-list

already shows aarch64-unknown-linux-musl.

It's also listed on the targets page here as supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm cross is just a low-setup way to cross compile. If we dont use cross then we have to provide the build toolchain manually for each target like its done here: https://github.com/eclipse-sdv-blueprints/fleet-management/blob/main/components/Dockerfile.fms-consumer

push:
branches:
- main
- add_build_and_containerize_pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed prior to merge, right? Just for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! its actually a bit of a problem though. We cant run the pipeline before merging it, so Im not really sure what the process of testing it before merge would be... By having it trigger on PR I could at least test that the github action itself runs through (although only on our forked repo), but not sure how I can test if the access to the main repos ghcr for example works as intended without merging to main.

mayyybe since you have more access rights here, you can manually trigger the workflow from the actions section? I did allow the "workflow_dispatch" trigger after all, but there seems to be some strange behavior in github where it might still not like being run manually...


name: Containerize uStreamer and Push to Container Registry

on:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be done on each PR? Or only when we have a release? Would like your feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally I guess only for proper releases? Until we have a real release strategy though maybe I can just set it to "workflow_dispatch" provided that actually works. Then we can manually release whenever we feel like it.

@@ -0,0 +1,164 @@
# ********************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of this I'm taking on faith it's being done correctly, since I'm not familiar enough with Docker and containerizing. 😅

A basic question though -- you've tested the resulting Docker image on your end to make sure it works?

Anyone else that is more familiar with Docker I'd be super appreciative if you could take a look! cc @AnotherDaniel @sophokles73

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tried around with the image and container by itself and found no issues yet. Im also using it in our work around the s2s blueprint and so far it behaves exactly like a streamer in a container should.

@@ -0,0 +1,28 @@
# ********************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't leave a comment on the .DS_Store file, so I'll leave it here.

Please add this and other "macOS-isms" as new entries in the .gitignore :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

untracked the macos nonsense. thanks for noticing ^^

# SPDX-License-Identifier: Apache-2.0
# *******************************************************************************/

[build.env]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to why you chose to use cross-rs? If we don't use cross-rs is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is the config for cross build. Instead of having to provide the whole toolchain directly we can use the default one for each target and just append it via the config here.

@@ -0,0 +1,23 @@
# ********************************************************************************
# Copyright (c) 2024 Contributors to the Eclipse Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reflect 2025 as the copyright year for any newly added files throughout as you've got a number of new files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! hope I didnt miss any

@@ -0,0 +1,10 @@
services:
ustreamer:
image: benediktillich/streamertest:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry again for my Docker-ignorance. Claude is telling me that:

benediktillich is the username or organization on Docker Hub

Should this be changed to something specific to Eclipse uProtocol? Do we need to make an account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah this was from testing. I removed this folder altogether. The example docker directory is now within the example-streamer-implementations folder and it should have the correct image names in the compose.yaml.

@@ -67,7 +67,7 @@ async fn main() -> Result<(), UStatus> {
if !args.endpoint.is_empty() {
// Specify the address to listen on using IPv4
let ipv4_endpoint =
EndPoint::from_str(args.endpoint.as_str()).expect("Unable to set endpoint");
EndPoint::from_str("tcp/127.0.0.1:7447").expect("Unable to set endpoint");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should respect if the user is choosing a different endpoint here.

Suggested change
EndPoint::from_str("tcp/127.0.0.1:7447").expect("Unable to set endpoint");
EndPoint::from_str(args.endpoint.as_str()).expect("Unable to set endpoint");

I see you've already set it as default, so if needed you can update the README.md to ensure that users are aware they should not override it if using the Docker-ized setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah that must have also been from testing. Thanks for catching all these...

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.

2 participants