compatible with k3d v5.x+ version #229
Conversation
LinuxSuRen
changed the title
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
=========================================
Coverage ? 10.43%
=========================================
Files ? 33
Lines ? 1447
Branches ? 0
=========================================
Hits ? 151
Misses ? 1283
Partials ? 13
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
kubectl-plugin/install/k3d.go
Outdated
| @@ -119,6 +138,21 @@ func (o *k3dOption) runE(cmd *cobra.Command, args []string) (err error) { | |||
| return | |||
| } | |||
|
|
|||
| //checkK3dVersion check if k3d version is greater than v5 | |||
| func checkK3dVersion(version string) (bool, error) { | |||
There was a problem hiding this comment.
it's pretty easy to add unit tests for this function. So, I'd like to suggest you add it.
There was a problem hiding this comment.
How about making the name of this function be more clear? Such as, isGreatThanV5 or isLargeThanV5.
It's hard to understand what checkK3dVersion will do by its name.
kubectl-plugin/install/k3d.go
Outdated
| return err | ||
| } | ||
|
|
||
| b, err := checkK3dVersion(out) |
There was a problem hiding this comment.
Please use a meaningful variable name instead of b. It's hard to understand what b represents for.
kubectl-plugin/types/constant.go
Outdated
| @@ -3,4 +3,8 @@ package types | |||
| const ( | |||
| // KsVersion is the latest release version | |||
| KsVersion = "v3.1.1" | |||
| // K3dVersion4 is the latest for k3d v4 | |||
| K3dVersion4 = "v4.4.8" | |||
There was a problem hiding this comment.
Are there any other use cases for this const? Currently, I can see that it is only useful for the unit test.
There was a problem hiding this comment.
No more for now, the version of k3d is obtained by local command, there is no place in our code to specify the version, whether to specify it as a constant
There was a problem hiding this comment.
In this case, I'd prefer to remove these const. They are only test variables or data.
kubectl-plugin/install/k3d.go
Outdated
| out, err := common.ExecCommandGetOutput("k3d", "version") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| isNewVersion, err := checkK3dVersion(out) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // init agent port adaptation k3d v4 | ||
| agentPort := "agent[0]" | ||
| // if k3d version is greater than v5, reset agent port | ||
| if isNewVersion { | ||
| agentPort = "agent:0" | ||
| } | ||
|
|
There was a problem hiding this comment.
I prefer to move these lines into a new function. But this comment does not bock other reviews.
shihaoH
force-pushed
the
k3dversioncompatible
branch
from
d9494e4
to
470c10c
Compare
fix: #227
References