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

CI: Update self-hosted arm runner to ubuntu-24.04-arm #268

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

singh1203
Copy link
Contributor

Fixes: #267

What changes are included in this PR?

Replaced the running platform from self-hosted arm runner to ubuntu-24.04-arm

Are these changes tested?

Not locally, but can be tested by triggering the runner

Are there any user-facing changes?

No

@singh1203 singh1203 changed the title (GH:apache#267) (ci)(chore) Update to ubuntu-24.04-arm CI: Update self-hosted arm runner to ubuntu-24.04-arm Feb 5, 2025
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
I would remove the if check for the GitHub repository owner and run on arm as we do for amd64. I mean removing:

          if [ "$GITHUB_REPOSITORY_OWNER" = "apache" ]; then
            echo "," >> "$GITHUB_OUTPUT"
            cat <<JSON >> "$GITHUB_OUTPUT"

and reorganize the code to a single JSON write.

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

LGTM

@kou
Copy link
Member

kou commented Feb 5, 2025

Could you remove the docker-targets job? Could you define matrix in the docker job statically? We don't need to define matrix dynamically now.

@raulcd
Copy link
Member

raulcd commented Feb 5, 2025

If you want an example from what @kou suggests you can see this diff on the main apache/arrow repo where the docker-targets was removed for a matrix:
https://github.com/apache/arrow/pull/45308/files

@singh1203
Copy link
Contributor Author

Sure @kou. I'd love to help here, and Thank you @raulcd for pointing me in right direction

@singh1203
Copy link
Contributor Author

singh1203 commented Feb 5, 2025

Could you remove the docker-targets job? Could you define matrix in the docker job statically? We don't need to define matrix dynamically now.

Done. Let's trigger the runner for testing.

Comment on lines +41 to +44
- arch: amd64
go: 1.22
runs-on: ubuntu-latest
arch-label: AMD64
Copy link
Member

Choose a reason for hiding this comment

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

Could you keep these keys in alphabetical order?

Suggested change
- arch: amd64
go: 1.22
runs-on: ubuntu-latest
arch-label: AMD64
- arch-label: AMD64
arch: amd64
go: 1.22
runs-on: ubuntu-latest

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.

Use ubuntu-24.04-arm instead of self-hosted arm runner in CI
3 participants