-
Notifications
You must be signed in to change notification settings - Fork 4
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-7007] Add Manifest Generation in CI #78
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, guessing you will need to test e2e, also see my note about windows support in filenames
fi | ||
|
||
# Set platform based on architecture in binary name | ||
if [[ "$BINARY_NAME" == *"x86_64"* ]]; then |
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 say this in the tone of the trailer for a horror movie but remember, windows is coming
linux/amd64 isn't the only amd64 anymore
I think you need to do one of:
- handle
*windows-x86_64
first, and make sure*x86_64
is an else case - always include OS in the filename
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 call. I'd been trying not to do ANYTHING windows here, to not conflict with your PR, but obviously new stuff in CI wouldn't be in your PR anyway.
Updated this to handle OS and Arch separately, so windows should work.
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're assuming I haven't created an entire parallel manifest system on my branch that will lead to terrible merge conflicts
Add manifest generation to agent builds, to support incoming work in App.