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

Enable ARM64 Workflow #128

Merged
merged 12 commits into from
Jan 20, 2024
Merged

Enable ARM64 Workflow #128

merged 12 commits into from
Jan 20, 2024

Conversation

tcchawla
Copy link
Contributor

Enables test runners to use ARM64 in the workflow. Fixes #122

Signed-off-by: tcchawla <[email protected]>
@tcchawla
Copy link
Contributor Author

Hey @jmhbnz, I have made the necessary changes, please approve the workflow to execute the pending jobs.

@jmhbnz
Copy link
Member

jmhbnz commented Jan 18, 2024

/ok-to-test

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Thanks @tcchawla a few points of feedback below.

.github/workflows/test_arm64.yaml Outdated Show resolved Hide resolved
.github/workflows/test_arm64.yaml Outdated Show resolved Hide resolved
.github/workflows/test_arm64.yaml Outdated Show resolved Hide resolved
.github/workflows/test_arm64.yaml Outdated Show resolved Hide resolved
@tcchawla
Copy link
Contributor Author

Hey @jmhbnz, I have addressed these review comments in the latest commit. Please verify.

@tcchawla tcchawla requested a review from jmhbnz January 19, 2024 00:14
@ivanvc
Copy link
Member

ivanvc commented Jan 19, 2024

@jmhbnz, would it make sense to introduce a template so ARM64/AMD64+i386 can reuse the same base and keep the workflow DRY?

@jmhbnz
Copy link
Member

jmhbnz commented Jan 19, 2024

@jmhbnz, would it make sense to introduce a template so ARM64/AMD64+i386 can reuse the same base and keep the workflow DRY?

That's a great suggestion, happy for it to be done in this pr or alternatively as a follow-up.

@tcchawla
Copy link
Contributor Author

I can do it in this PR if that's okay

@tcchawla
Copy link
Contributor Author

Hey @jmhbnz, I have added a template file and made changes to the workflows test files. Let me know if any changes are required.

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Hey @tcchawla - If you take a look at the actions run you will see some errors: https://github.com/etcd-io/raft/actions/runs/7586269735

 Invalid workflow file: .github/workflows/test_arm64.yaml#L6
The workflow is not valid. In .github/workflows/test_arm64.yaml (Line: 6, Col: 11): Error from called workflow etcd-io/raft/.github/workflows/test_template.yaml@11d03fd542a2cc6c8cd869e5a45b1634ef9261a1 (Line: 21, Col: 17): Unrecognized named-value: 'input'. Located at position 10 within expression: fromJSON(input.targets) In .github/workflows/test_arm64.yaml (Line: 6, Col: 11): Error from called workflow etcd-io/raft/.github/workflows/test_template.yaml@11d03fd542a2cc6c8cd869e5a45b1634ef9261a1 (Line: 21, Col: 17): Unexpected value '${{ fromJSON(input.targets) }}'

Looks like the templating is not quite right for the targets value. Please give it another attempt and feel free to message me or @ivanvc on kubernetes slack if you get stuck.

Signed-off-by: tcchawla <[email protected]>
Signed-off-by: tcchawla <[email protected]>
Signed-off-by: tcchawla <[email protected]>
@tcchawla
Copy link
Contributor Author

Hey @jmhbnz, turns out it was a typo. I have pushed the working code.

@tcchawla tcchawla requested a review from jmhbnz January 19, 2024 21:16
Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

Hi @tcchawla and @jmhbnz, I took a pass on this PR. Hope it's fine ✌️

.github/workflows/test_arm64.yaml Outdated Show resolved Hide resolved
.github/workflows/test_template.yaml Outdated Show resolved Hide resolved
Signed-off-by: tcchawla <[email protected]>
Copy link
Member

@ivanvc ivanvc 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 changes. I defer to @jmhbnz and an active project maintainer :)

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @tcchawla and @ivanvc

@jmhbnz jmhbnz requested a review from ahrtr January 20, 2024 07:50
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks all

@ahrtr ahrtr merged commit e22adc0 into etcd-io:main Jan 20, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable ARM64 GitHub workflows
5 participants