-
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-7885: Send Event to Run Networking Tests on Release Candidate #4083
RSDK-7885: Send Event to Run Networking Tests on Release Candidate #4083
Conversation
- name: Trigger Workflow | ||
run: | | ||
curl -X POST \ | ||
-H "Authorization: token ${{secrets.GITHUB_TOKEN}}" \ |
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'm not sure if this is the right token needed to be able to run this curl, but it does need to have multiple repo write access to work. Theoretically with the token I could run this command to see if it triggers the workflow as well
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're gonna have to test this offline; let's check in with some combination of @abe-winter + @maximpertsov.
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.
another testing option -- make this its own file, give it a workflow_dispatch event, and run manually
(no harm in running extra sdk tests)
I have no preference though, do whatever works best for sdk team.
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.
Nice!
- name: Trigger Workflow | ||
run: | | ||
curl -X POST \ | ||
-H "Authorization: token ${{secrets.GITHUB_TOKEN}}" \ |
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're gonna have to test this offline; let's check in with some combination of @abe-winter + @maximpertsov.
-H "Authorization: token ${{secrets.GITHUB_TOKEN}}" \ | ||
-H "Accept: application/vnd.github.everest-preview+json" \ | ||
https://api.github.com/repos/viamrobotics/sdk-integration-tests/dispatches \ | ||
-d '{"event_type":"new_rc_branch"}' |
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.
[nit] Missing newline at EOF here?
@@ -67,3 +67,17 @@ jobs: | |||
slack_webhook_url: ${{secrets.SLACK_WEBHOOK_URL}} | |||
channel: '#team-devops' | |||
name: 'Workflow Status' | |||
|
|||
run-sdk-networking-tests: | |||
needs: [test, staticbuild, appimage] |
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.
[nit + q] I don't think this "needs" test
, as both of staticbuild
and appimage
"need" it already (i.e. it's transitively "needed"). Also, does appimage
finishing ensure that the new RC builds will be available to test against from the SDK integration tests?
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 believe so as the last step of the appimage
workflow is to output links to the newly uploaded RC builds, and this job won't run until appimage
is successful
run: | | ||
curl -X POST \ | ||
-H "Authorization: token ${{secrets.GITHUB_TOKEN}}" \ | ||
-H "Accept: application/vnd.github.everest-preview+json" \ |
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.
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.
Spoke offline with @abe-winter, but I re-iterate that it's a little crazy that GHCI has no "builtin" way of doing this...
-H "Accept: application/vnd.github+json" \ | ||
https://api.github.com/repos/viamrobotics/sdk-integration-tests/dispatches \ | ||
-d '{"event_type":"new_rc_branch"}' | ||
|
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.
Something's up with your editor here, as there's still no terminating newline. Maybe try https://stackoverflow.com/questions/44704968/visual-studio-code-insert-newline-at-the-end-of-files?
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.
thanks for the tip! will never have this issue again 😼
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 as long as we're confident the secrets.REPO_READ_TOKEN
will correctly authorize the curl 🧑🔧 .
If it doesn't, will this workflow be marked as a failure? And if so, does that mess up any release workflows downstream? @abe-winter |
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.
would normally say 'merge + test in prod' but it would be bad to break the RC workflow
imo it's worth finding a way to test this that isn't 'run an RC' (RCs are kind of stateful because of the go sumdb)
recommendation: make this its own yaml file with on: workflow_call
and workflow_dispatch
, merge to main, run manually + see what happens; then invoke via workflow call from releasecandidate.yml file
(can talk through live if helpful)
7022470
to
578ae66
Compare
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.
The PR looks good pending testing suggested here #4083 (review)
Description
Testing
sdk-integration-tests
repo: