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 integration tests for arm64 #555

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

dharmit
Copy link
Contributor

@dharmit dharmit commented Aug 8, 2024

No description provided.

@dharmit dharmit requested a review from a team as a code owner August 8, 2024 10:15
@dharmit dharmit force-pushed the fix-r/r-45420 branch 3 times, most recently from 3d61844 to aa8d189 Compare August 8, 2024 12:46
Copy link
Member

@mallardduck mallardduck left a comment

Choose a reason for hiding this comment

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

I think once the OS fields have both values the action will work as expected. 👍

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@mallardduck
Copy link
Member

Since i've just pushed our new main branch, I've repointed this PR to target main.
It should be a smooth transition since main is based on release/v5.0.

@mallardduck
Copy link
Member

Ok @dharmit - I put some more dots together here in this puzzle for us.
The labels we saw Alex working with weren't wrong, just 4 months old before our full adoption of GHA.
And things must have changed since then by a lot since our new docs look a bit different: https://github.com/rancherlabs/eio/wiki/GitHub-Actions-Runners

@dharmit
Copy link
Contributor Author

dharmit commented Aug 9, 2024

And things must have changed since then by a lot since our new docs look a bit different: https://github.com/rancherlabs/eio/wiki/GitHub-Actions-Runners

They don't mention arm64 in containerized runners section. All occurrences of arm64 are under the "Non-Containerized Org-Level Runners (VMs and Bare Metal Instances)" section. 🤔

Also that doc doesn't mention "ubuntu-latest" anywhere, and yet we are using it in our tests.

@mallardduck
Copy link
Member

I think we just need to try: https://github.com/rancherlabs/eio/wiki/GitHub-Actions-Runners#containerized-org-level-runners

And I suspect that they have arm ones ready to go?

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@dharmit dharmit force-pushed the fix-r/r-45420 branch 3 times, most recently from 5855019 to 2f9f0e6 Compare August 9, 2024 13:24
@mallardduck
Copy link
Member

@dharmit - Disregard my suggestion to use a new CI file, I've worked with EIO to get this repo access to non-container runners. This provides us a much more direct drop-in replacement for the GHA runners. With those setup I was able to tweak this PR to use those runners for both arm and amd64 without major changes.

@mallardduck mallardduck self-requested a review August 9, 2024 16:23
Copy link
Member

@mallardduck mallardduck left a comment

Choose a reason for hiding this comment

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

Still have some changes to be made before this works, I will leave these for you to resolve though now that we know the runners are sorted out and working. 😄

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@mallardduck
Copy link
Member

Might have to run workflows as debug to get additional logging. Looks like we're getting close but now seeing a new error related to no ARCH set and go binary missing.

@dharmit
Copy link
Contributor Author

dharmit commented Aug 14, 2024

Looks like we're getting close but now seeing a new error related to no ARCH set and go binary missing.

I think the two are related because ARCH is set in ./scripts/version as:

ARCH=$TARGET_ARCH
if [ -z "$ARCH" ]; then
    ARCH=$(go env GOHOSTARCH)
fi

Since go binary isn't available, both problems are popping up. Let me try to install go.

set -x

GO_VERSION=1.22.0
ARCH=`uname -i`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use uname -m instead. -i isn't supported on macOS and is documented as non-portable on ubuntu. As much as I can tell, they do the same thing.

GO_VERSION=1.22.0
ARCH=`uname -i`

if [ $ARCH -eq "x86_64" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's clearer to set PLATFORM=amd64|arm64 in a separate case block to avoid duplicating a long command.

But you should probably set this in ci.yaml and set the variables if they're unset so .github/scripts/X can still be run outside GHA, mostly for testing/dev purposes.

@@ -0,0 +1,16 @@
#!/bin/bash

set -e
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead do

set -eu
set -x

where the set -x could get dropped at a later date. And our scripts should always fail on unset vars

Copy link
Collaborator

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

I would drop the entire install-go.sh file and replace it by running actions/setup-go in ci.yaml

Also see how we deal with the two different ways of naming the amd architecture in webhook/.../ci.yaml. And I would then try to pass a value for ARCH to install-mc.sh and for TARGET_ARCH for ./scripts/integration

@dharmit dharmit force-pushed the fix-r/r-45420 branch 2 times, most recently from 31c8cb7 to 2e3990a Compare August 19, 2024 10:31
Signed-off-by: Dharmit Shah <dharmit.shah@suse.com>
@dharmit
Copy link
Contributor Author

dharmit commented Aug 19, 2024

I would drop the entire install-go.sh file and replace it by running actions/setup-go in ci.yaml

Thanks. I wasn't aware of it. 👍🏽

Also see how we deal with the two different ways of naming the amd architecture in webhook/.../ci.yaml. And I would then try to pass a value for ARCH to install-mc.sh and for TARGET_ARCH for ./scripts/integration

I haven't made any changes that you suggested here. And, I think, it didn't cause any trouble because go installation happened before we run integration tests.

@mallardduck mallardduck merged commit 67af72e into rancher:main Aug 20, 2024
6 checks passed
@mallardduck
Copy link
Member

/backport NONE release/v5.0

@dharmit dharmit deleted the fix-r/r-45420 branch August 21, 2024 06:38
mallardduck added a commit to mallardduck/backup-restore-operator that referenced this pull request Aug 30, 2024
Add integration tests for arm64

(cherry picked from commit 67af72e)
mallardduck added a commit to mallardduck/backup-restore-operator that referenced this pull request Aug 30, 2024
Add integration tests for arm64

(cherry picked from commit 67af72e)
mallardduck added a commit to mallardduck/backup-restore-operator that referenced this pull request Aug 30, 2024
Add integration tests for arm64

(cherry picked from commit 67af72e)
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.

3 participants