Explicitly restrict to amd64 hosts

[tiferrei]

Until ARM support is ready, I have been running this version on my mixed cluster to make sure that only amd64 nodes serve the deployments.

[phlogistonjohn]

Thanks for the patches! One quick comment: we use a commit message style similar to the kernel where the "topic" (as in topic: desc) should be an area of the codebase (basically subdirs). So in your case that would be "config:". Thanks!

[tiferrei]

So in your case that would be "config:". Thanks!

Sure! Can this done at the squash & merge point?

[phlogistonjohn]

That's not a workflow that I usually follow, but if you're not comfortable doing it yourself, I can look into it, for sure.

[tiferrei]

Sorry I may be misunderstanding something here.

This seems to be specific to the commit names in my fork, which don't necessarily carry onto the main repo in a PR, depending on the merge method. Would you prefer that I change the commit messages in my fork to align with the main repo?

[phlogistonjohn]

No worries, I am used to working on projects (on github) that resolve a PR with either "Rebase and merge" or "Create a merge commit". On this project we rebase. With these approaches the submitter's commit messages are preserved. I am aware of the squash option but I have not used it myself. I'm used to simply asking the submitter to update the commit messages. However, I don't require it as you enabled 'Maintainers are allowed to edit this pull request' - so if all else fails I can just fix up the commits myself prior to merge.

On the topic of commit messages, we also prefer to have the "Signed-off-by" line present. This is not something I would add/change for you: https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for

In case you haven't seen it yet, the CI's yaml linter has detected issues:
https://github.com/samba-in-kubernetes/samba-operator/actions/runs/7791303609/job/21326450694

[tiferrei]

Aah okay I understand, sorry I actually just was not aware of the use of this convention. I am very happy to do this I just don't have all the details. Would it be helpful if I change the commit(s) above into one with the message, for example but feel free to edit, 'config: restrict to amd64 nodes through affinity'? I can certainly do something like this!

Additionally I will fix the indentation!

[phlogistonjohn]

The CI is failing due to golang related issues that ought to be unrelated to this PR. I wonder if it's triggered by the recent go release?

Error: /home/runner/work/samba-operator/samba-operator/internal/resources/smbshare_test.go:165:4: parameter 'ctx' seems to be unused, consider removing or renaming it as _
make: *** [Makefile:219: check-revive] Error 1
Error: /home/runner/work/samba-operator/samba-operator/internal/resources/smbshare_test.go:201:4: parameter 'ctx' seems to be unused, consider removing or renaming it as _
Error: /home/runner/work/samba-operator/samba-operator/internal/resources/smbshare_test.go:202:4: parameter 'nn' seems to be unused, consider removing or renaming it as _
Error: /home/runner/work/samba-operator/samba-operator/internal/resources/smbshare_test.go:203:4: parameter 'obj' seems to be unused, consider removing or renaming it as _

Regardless of the cause for this, I pull the PR locally and ran the YAML lint checks again and it complains about the indent for two lines. These indents can be fixed by the following:

diff --git a/config/manager-full/auth_proxy_patch.yaml b/config/manager-full/auth_proxy_patch.yaml
index 3b39da3..154e40a 100644
--- a/config/manager-full/auth_proxy_patch.yaml
+++ b/config/manager-full/auth_proxy_patch.yaml
@@ -13,7 +13,7 @@ spec:
           requiredDuringSchedulingIgnoredDuringExecution:
             nodeSelectorTerms:
               - matchExpressions:
-                - key: beta.kubernetes.io/arch
+                  - key: beta.kubernetes.io/arch
                 operator: In
                 values:
                   - amd64
diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml
index 593028d..819880d 100644
--- a/config/manager/manager.yaml
+++ b/config/manager/manager.yaml
@@ -36,7 +36,7 @@ spec:
           requiredDuringSchedulingIgnoredDuringExecution:
             nodeSelectorTerms:
               - matchExpressions:
-                - key: beta.kubernetes.io/arch
+                  - key: beta.kubernetes.io/arch
                 operator: In
                 values:
                   - amd64

If you make these updates I think I'd be OK with bypassing the failing CI checks for the Go code.

[phlogistonjohn]

See previous comment.

[tiferrei]

Should be good now!

[phlogistonjohn]

Changes look OK but the new patch lacks a sign off by, etc. If you prefer I squash them together I can do so later after. Is that what you'd prefer?

[tiferrei]

Sorry, now amended properly.

[synarete]

Those two commits should be squashed into one. See how I did it in my (untested) branch: synarete@fa85754

[synarete]

key should be kubernetes.io/arch. See k8s labels-annotations-taints docs as well kubernetes/kubernetes#73333

Note that we already use those values for samba pods' selector: https://github.com/samba-in-kubernetes/samba-operator/blob/master/internal/planner/selector.go#L14

[synarete]

Same comment as above: should be kubernetes.io/arch

[tiferrei]

Sorry, I have given more time to this PR than I can reasonably dedicate.

I use my fork on my cluster, I have no practical motivation to keep investing time in these changes, or squashing and renaming commits that make no difference to me.

I love the project, if you'd like to have the changes upstreamed you are welcome to do any edits you'd like, or close the PR if it is not of value.

[mergify]

This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch.

[obnoxxx]

@tiferrei thanks again for your contribution! This has been merged as PR #328