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

explicit tools versions #248

Closed synarete closed 1 year ago

synarete commented 1 year ago

Currently, samba-operator supports as far back as Go1.16 (e.g., in github actions). However, not all our build tools are compatible with this version in their latest release. Thus, we need to pin-point each tool to explicit version which is not too old but still supports Go1.16.

phlogistonjohn commented 1 year ago

Currently, samba-operator supports as far back as Go1.16 (e.g., in github actions). However, not all our build tools are compatible with this version in their latest release. Thus, we need to pin-point each tool to explicit version which is not too old but still supports Go1.16.

This is somewhat of a historical accident. In my mind the policy is not "support Go 1.16" it is "support all the current versions of Go". Therefore in a number of cases we can't say "latest" because that would only build/test with the newest version of Go and we run the risk of using stuff that won't work on Go. If there was a symbolic name in the setup-go action, I'd probably use it... but I can't find such a thing so we're stuck bumping up the version numbers from time to time.

Sometimes, when I'm letting my distribution lag and I'm using an older version on my dev machine(s) I don't mind the CI lagging along with me. However, that's out of laziness and I can usually be talked out of that without much effort. :-)

All this said, I think this change is a fine start and there's nothing wrong with this!

phlogistonjohn commented 1 year ago

@synarete @anoopcs9 how do we want to handle the centos ci failures for a change like this. I'm fine with merging it w/o a passing centos ci run as it's fairly small and mainly directed at the github checks, which passed. Any thoughts?

synarete commented 1 year ago

@synarete @anoopcs9 how do we want to handle the centos ci failures for a change like this. I'm fine with merging it w/o a passing centos ci run as it's fairly small and mainly directed at the github checks, which passed. Any thoughts?

As far as I can tell from CI log, those failures are not related to the actual changes in the PR. @anoopcs9 Could you please take a look as well? (just to be on the safe side).

anoopcs9 commented 1 year ago

@synarete @anoopcs9 how do we want to handle the centos ci failures for a change like this. I'm fine with merging it w/o a passing centos ci run as it's fairly small and mainly directed at the github checks, which passed. Any thoughts?

As far as I can tell from CI log, those failures are not related to the actual changes in the PR. @anoopcs9 Could you please take a look as well? (just to be on the safe side).

As informed via other channels, TestIntegration/smbSharesClustered/withDNS/TestShareAccessByDomainName started failing for every run after recent migration to reserve nodes for testing from new EC2 pool in CentOS CI infra. This is currently under investigation #252

phlogistonjohn commented 1 year ago

I appreciate the update. Getting back to my original question: regardless of the cause and severity of the current centos ci issues, does anyone mind just merging this without waiting for the ci issues to be resolved? I think it's small scoped enough to do so, personally.

synarete commented 1 year ago

Can we have the following typos fixed in commit messages?

Sure, will fix