search

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
103 stars 24 forks source link

workflows: build-only for x86_64 and aarch64 #257

Closed synarete closed 1 year ago

synarete commented 1 year ago

Provide build-only workflow which executes minimal 'make && make check && make test' for every supported architecture (namely, x8664/amd64 and aarch64/arm64). This workflow is triggered for every push on any non-private branch (that is, branch-name does not begin with '') in order to ensure that build process did not break. In particular, that all build-tools work properly on architectures other than the one used by developer.

Signed-off-by: Shachar Sharon ssharon@redhat.com

synarete commented 1 year ago

Seems OK, generally. It does duplicate a bit of the build steps in the other CI yaml file, but that's probably fine for now until there's more support for both arch's in the whole SINK stack.

IMHO this is complementary to CI jobs. It is helper for developer to ensure he did not break something which he could not detect on his local setup. As a matter of fact, I would also like to add to this PR strategy.matrix.go: ["1.17", "1.18"]

phlogistonjohn commented 1 year ago

Seems OK, generally. It does duplicate a bit of the build steps in the other CI yaml file, but that's probably fine for now until there's more support for both arch's in the whole SINK stack.

IMHO this is complementary to CI jobs. It is helper for developer to ensure he did not break something which he could not detect on his local setup. As a matter of fact, I would also like to add to this PR strategy.matrix.go: ["1.17", "1.18"]

Sure. I don't want to belabor the point much more because I am ready to approve the change, but what I was trying to say is once we can, I'd think we'd want to fold these tests into the other tests and test everything (build and functionality) on all architectures we want to support. :-)

And yes to expanding the matrix of go versions.

phlogistonjohn commented 1 year ago

I started reading over the docs related to the matrix build and now I am sorry to say I do not understand how this executes an aarch64 build. Based on my readings of the docs the matrix.include subsection is useful for "filling in the blanks" https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations

I don't see that this implicitly sets the architecture of the builder. Then I went a-searching for more information about having the github runner use aarch64 and found nothing beyond self-hosted runners. I'm now worried that this doesn't do what we think it does and in fact the job we think runs on aarch64 will run on x86 instead. Short of just merging this to see what happens, which I'm OK with in the short term, is there a way to test this is actually running on the architecture indicated?

synarete commented 1 year ago

I started reading over the docs related to the matrix build and now I am sorry to say I do not understand how this executes an aarch64 build. Based on my readings of the docs the matrix.include subsection is useful for "filling in the blanks" https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations

I don't see that this implicitly sets the architecture of the builder. Then I went a-searching for more information about having the github runner use aarch64 and found nothing beyond self-hosted runners. I'm now worried that this doesn't do what we think it does and in fact the job we think runs on aarch64 will run on x86 instead. Short of just merging this to see what happens, which I'm OK with in the short term, is there a way to test this is actually running on the architecture indicated?

@phlogistonjohn You are correct. Digging deeper into github's docs. I realized that for the time being, ARM64 is supported only for self-hosted-runners, which is not our case. I will change this PR to be a draft; maybe one day github will also support ARM64 runners as well.

phlogistonjohn commented 1 year ago

I hope you don't mind, but I'd prefer that we close it rather than make it a draft. We should file an issue to track the need to testing arm64 builds.

If we knew this was on GH's roadmap and going to be updated in Nov, or maybe even Dec. I'd be cool with leaving it open as a draft, I just don't like going to the project and seeing a pr open that I don't know if ever will make progress. Does that make sense to you?

synarete commented 1 year ago

I hope you don't mind, but I'd prefer that we close it rather than make it a draft. We should file an issue to track the need to testing arm64 builds.

If we knew this was on GH's roadmap and going to be updated in Nov, or maybe even Dec. I'd be cool with leaving it open as a draft, I just don't like going to the project and seeing a pr open that I don't know if ever will make progress. Does that make sense to you?

I agree. My apologies for this -- I should have dived deeper into GH docs before submitting this PR.

phlogistonjohn commented 1 year ago

I agree. My apologies for this -- I should have dived deeper into GH docs before submitting this PR.

No worries & no need to apologize. I wish GH had enabled arm runners, I was very excited to take this but once I started to look into some of the details I discovered that there were no built-in arm runners yet.

phlogistonjohn commented 1 year ago

Closing as discussed.