Closed twz123 closed 1 year ago
Will there be some panic b/c the version is nil?
The validator should catch that. "Version: cannot be blank" comes from the validation.Required
check.
func (k *K0s) Validate() error {
return validation.ValidateStruct(k,
validation.Field(&k.Version, validation.Required),
validation.Field(&k.Version, validation.By(validateVersion)),
validation.Field(&k.DynamicConfig, validation.By(k.validateMinDynamic())),
)
}
func validateVersion(value interface{}) error {
v, ok := value.(*version.Version)
if !ok {
return fmt.Errorf("not a version")
}
if v != nil && !k0sSupportedVersion.Check(v) {
return fmt.Errorf("minimum supported k0s version is %s", k0sSupportedVersion)
}
return nil
}
Not sure what the alternative for the functionality would be 🤔
Try to dig it from the binary without running it
$ bin=~/Library/Caches/k0sctl/k0s/linux/amd64/k0s-1.28.2+k0s.0; \
str="github.com/k0sproject/k0s/pkg/build.Version="; \
offset=$(grep -abo ${str} ${bin}|head -1|cut -d":" -f1); \
dd if=${bin} bs=1 skip=$((offset+${#str})) count=15 2>/dev/null | cut -d" " -f1
v1.28.2+k0s.0
Adding a default version is perfectly reasonable IMO, but maybe it could happen at a different stage in the code? I wouldn't try to be smarter about the version than we currently are. If the internet lookup fails, I would expect k0sctl to print an appropriate error message and exit. This would make it understandable for folks, and if they use k0sctl in an airgapped environment, they can simply specify the version in the config themselves.
Currently, the error that occurs when fetching the current version is discarded, so it's not directly obvious why the version is sometimes blank, and sometimes not. Maybe it would be okay to simply propagate the error somehow and let the unmarshalling fail? On the other hand, unmarshalling a struct from YAML feels like an operation that should probably be idempotent.
Moving this to a different place in the code could also help with the unit tests. The new code could have some means of getting the current version injected, so that it's testable without internet.
The k0sctl unit tests fail when executed in sandboxed environments without internet connection. This is due to the fact that k0sctl tries to fetch the most recent k0s version from the internet if it's not specified in the config.
Test failures
```text --- FAIL: TestUnmarshal (0.00s) --- FAIL: TestUnmarshal/version_not_given (0.00s) k0s_test.go:33: Error Trace: /build/source/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/k0s_test.go:33 Error: Received unexpected error: Version: cannot be blank. Test: TestUnmarshal/version_not_given --- FAIL: TestVersionDefaulting (0.00s) --- FAIL: TestVersionDefaulting/version_not_given (0.00s) k0s_test.go:48: Error Trace: /build/source/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/k0s_test.go:48 Error: Received unexpected error: Version: cannot be blank. Test: TestVersionDefaulting/version_not_given FAIL FAIL github.com/k0sproject/k0sctl/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster 0.025s FAIL ```I wonder what happens if there's some intermittent network failure during the execution of k0sctl that leads to the version not being defaulted. Will there be some panic b/c the version is nil?