Loose kustomize version requirements #4994
Conversation
|
/Assign @MarlonGamez |
yuwenma
added
deploy/kpt
kind/bug
Codecov Report
@@ Coverage Diff @@
## master #4994 +/- ##
==========================================
+ Coverage 72.17% 72.21% +0.04%
==========================================
Files 363 363
Lines 12750 12760 +10
==========================================
+ Hits 9202 9215 +13
+ Misses 2864 2862 -2
+ Partials 684 683 -1
Continue to review full report at Codecov.
|
yuwenma
force-pushed
the
kustomize-no-verison-limit
branch
from
7606e4d to
01749a0
Compare
| @@ -116,15 +124,12 @@ func versionCheck(dir string) error { | |||
| re := regexp.MustCompile(kustomizeVersionRegexP) | |||
| match := re.FindStringSubmatch(versionInfo) | |||
| if len(match) != 2 { | |||
| return fmt.Errorf("unknown kustomize version %v\nPlease upgrade your "+ | |||
There was a problem hiding this comment.
WDYT about moving this kustomize version parsing logic into the kustomize package, similar to how we do kubectl.CompareVersionTo() checks? And similarly extracting the kpt version checking too? Then the version-checking can be tested in isolation and not involve string matching against the warning text.
There was a problem hiding this comment.
Very reasonable suggestion. We can add similar logic to kustomize package, but we may want to separate the version check from kpt. This is because we will replace the kustomize CLI to builtin kustomize package in kpt. This is pending until the unified kpt/kustomize design is done in the kpt team.
| kustomizeFurtherGuidance = fmt.Sprintf("Please make sure your local "+ | ||
| "kustomize version >= the official version %v, otherwise some features may not be "+ | ||
| "well supported. You can download the official version "+ | ||
| "from %v", kustomizeMinVersion, kustomizeDownloadLink) |
There was a problem hiding this comment.
I don't think it makes sense to have this text as a var since the arguments as positional instead of named. I think you're assigning this as a var to be used from the tests.
@tejal29 @PriyaModali here's a possible contribution to build failures that we should report upwards in our metrics: incompatible dependency versions. We have this for kubectl, helm, and now kpt and kustomize.
There was a problem hiding this comment.
I don't have strong opinions on the var usage. Yeah, this is just to save some lines in the unitests.
There was a problem hiding this comment.
Just FYI, for the incompatible dependency versions, I guess it is fine to ignore the conflict if users use kustomize in kpt deployer (like this PR only warns users but doesn't have hard requirements on actions) . This is because we will replace the kustomize here to some kpt builtin functions, so this version check may likely be changed someday in Q1 2021.
There was a problem hiding this comment.
Ack @briandealwis .
This is something i discussed with @tstromberg internally. Skaffold needs to provide a framework or mechanism to error out with an error code instead of playing this catch up game for new error messages.
Minikube has this implemented here
I am working on this idea. Happy to hear any thoughts.
Co-authored-by: Brian de Alwis <bsd@acm.org>
|
I think this is looking good now. +1 to Brian's idea about separating the kustomize version checking into the kustomize package, but that should probably be in a different PR. I'll run kokoro and wait on the result |
Fixes: #4981 #3904
Description
We warn users if kustomize version is non-official or too old. We don't block users using an old kustomize version since we believe kpt can better deal with the kustomize versioning once using a builtin kustomize .