-
Notifications
You must be signed in to change notification settings - Fork 110
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
RSDK-2996 Release Candidate Workflows #2336
Conversation
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 but left some comments you may want to address
|
||
steps: | ||
- name: Check out main branch code |
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.
🙏
- name: Upload Files (Testing) | ||
if: github.event_name == 'workflow_dispatch' || github.event_name == 'push' | ||
uses: google-github-actions/[email protected] | ||
with: | ||
headers: "cache-control: no-cache" | ||
path: 'etc/packaging/appimages/deploy/' | ||
destination: 'packages.viam.com/apps/viam-server/testing/appimage/${{ steps.build_test_app.outputs.date }}/${{ github.sha }}/' | ||
destination: 'packages.viam.com/apps/viam-server/testing/appimage/${{ steps.build_date.outputs.date }}/${{ github.sha }}/' |
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.
For when we publish things, would you mind using https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/ so it's easy to click Summary on the action?
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 didn't even know about that GITHUB_STEP_SUMMARY file. Cool stuff! I've added output to provide links to any built binaries. Two paths basically, one when PR artifacts are built (as that bails early) and another for any push-based triggers (tags or main updates.) Examples below:
Test run (PR triggered)
https://github.com/viamrobotics/rdk/actions/runs/4930753109
Test run (push triggered)
https://github.com/viamrobotics/rdk/actions/runs/4930749546
@@ -106,10 +117,14 @@ jobs: | |||
|
|||
- name: Test AppImage | |||
run: | | |||
channel="${{ github.ref_name }}" | |||
if [ "$channel" = "main" ]; then | |||
channel="latest" |
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.
short comment of why we do this here would be helpful
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.
Added # we call our main branch releases "latest"
@@ -162,7 +162,7 @@ require ( | |||
github.com/esimonov/ifshort v1.0.4 // indirect | |||
github.com/ettle/strcase v0.1.1 // indirect | |||
github.com/fatih/camelcase v1.0.0 // indirect | |||
github.com/fatih/color v1.14.1 // indirect |
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.
can probably undo these two go files for this PR
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.
Actually it was required to update actionlint otherwise it failed to pass the new workflows due to this bug: rhysd/actionlint#263
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.
ah okay
@@ -5,7 +5,7 @@ TOOL_BIN = bin/gotools/$(shell uname -s)-$(shell uname -m) | |||
PATH_WITH_TOOLS="`pwd`/$(TOOL_BIN):`pwd`/node_modules/.bin:${PATH}" | |||
|
|||
GIT_REVISION = $(shell git rev-parse HEAD | tr -d '\n') | |||
TAG_VERSION?=$(shell etc/tag_version.sh) | |||
TAG_VERSION?=$(shell git tag --points-at | sort -Vr | head -n1) |
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.
woo
@@ -25,12 +23,16 @@ jobs: | |||
appimage: | |||
needs: test | |||
uses: viamrobotics/rdk/.github/workflows/appimage.yml@main | |||
with: |
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.
is there a way to set this once in the overall workflow?
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 tried with env variables, but they don't pass through. Only way I could find was to use inputs to each workflow like this (same as we have to do with secrets.)
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.
ah no worries then!
|
This updates workflows to support release candidate builds. Main changes are that each "type" of release has its own parent workflow now (main/stable/rc/pr) and sets an environment variable to match, to make it easier to do conditional logic in child workflows.
Includes is also some simplification of flow and security fixups for the license_finder flow (which was running directly on pull request!)
Manually tested against both stable and rc tags (from another branch) as well as PR's against this test branch, and pushes to this branch as "main." (SMURF tags/renames using during testing have already been reverted to simplify review.)