-
Notifications
You must be signed in to change notification settings - Fork 7
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
Don't use canon in CI #57
Conversation
2f7f9aa
to
12d3607
Compare
12d3607
to
f99a764
Compare
|
||
- name: Upload viamrtsp module to registry | ||
uses: viamrobotics/upload-module@main | ||
uses: viamrobotics/upload-module@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what the docs recommend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know, I've been using main
…ild integration test module for testing
82904a4
to
c165af2
Compare
c165af2
to
164528f
Compare
Subbing martha for myself so we get more build action knowledge across module-heavy teams. |
|
||
target_os: linux | ||
target_arch: ${{ matrix.platform.arch }} | ||
docker_image: ${{ matrix.platform.docker_image }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't still need the go version? does docker already use a specific version so you don't need it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We specify using: 'composite'
in the action.yml, so the setup go action carries over to the environment of the main workflow
go-version: 1.21.13 | ||
|
||
- name: Pull Docker image | ||
run: docker pull ${{ inputs.docker_image }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so using with: and including a docker image in the other workflows makes that an input here? i'm not familiar with inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think of it as params and args
params being what's defined above in inputs:
and with:
being the args passed along
echo "module.tar.gz exists" | ||
else | ||
echo "module.tar.gz does not exist" | ||
exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the goal here is just to report if module.tar.gz exists? will this cause the workflow to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had this in for debugging but decided to keep it since it made sanity checking the existence of the make module
module.tar.gz
artifact easier. I think we can remove though it's not critical
@@ -14,11 +14,9 @@ jobs: | |||
steps: | |||
- uses: actions/checkout@v3 | |||
|
|||
- name: Set up Go | |||
uses: actions/setup-go@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be issues that you were using setup-go@v2
and now in action.yml
you're using setup-go@v3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point we don't need to bump it
reverted to v2
I’m still not super knowledgeable about these workflows, so I’d say def check on it after you merge to make sure there’s no issues, but as far as I can tell, looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Nothing to add here - really like the consolidated build flow :)
0.2.0-rc1 does not work on bullseye. This is likely because
canon
is being annoying in the CI runner and the artifact was not actually built insideviam-rtsp-antique
. The workaround is to directly use docker from the source RDK build imagesAdded new debugging step to make sure the module.tar.gz successfully builds
Consolidated build module logic in CI to use a single DRY reusable action
build-module.yml
that builds it in the actual build container used to publish modules in to unite build and test environments