Refactoring lifecycle package and dependencies #148
Conversation
pkg/lifecycle/lifecycle.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| errorChartRepository error = errors.New("chart repository is in an inconsistent state") |
There was a problem hiding this comment.
You don't have to indicate type here. The compiler will infer it. The name should be errChartRepository.
pkg/lifecycle/lifecycle.go
Outdated
| // Commit after each chart removal. | ||
| removedAssetsVersions, err := ld.removeAssetsVersions(debug) | ||
| if !exists { | ||
| return errorChartRepository |
There was a problem hiding this comment.
Is this error used only here? If so, you don't need to predeclare it above.
There was a problem hiding this comment.
There's a few other places that uses this error, and IMO we should declare every custom error, so when we unit test we have a variable to compare the expected error with the returned one.
There was a problem hiding this comment.
The error is used in 3 different places for now.
@briandowns
It is a general one that will be thrown if you have done something really nasty to the charts repository local folder.
That is why I am not doing 3 distinct errors and only a general one.
@rafaelbreno
pkg/lifecycle/parser_test.go
Outdated
| if dep.assetsVersionsMap["key1"][0].Version != "0.0.1" || | ||
| dep.assetsVersionsMap["key1"][1].Version != "0.1.0" || | ||
| dep.assetsVersionsMap["key1"][2].Version != "1.0.0" { | ||
| if dep.AssetsVersionsMap["key1"][0].Version != "0.0.1" || |
There was a problem hiding this comment.
We don't need to change this now nor do I know if you even wrote this but for reference, Hungarian notation in Go is a code smell and should be avoided.
There was a problem hiding this comment.
Fixed and also implemented assert package for unit-testing, it is easier to read. @briandowns
pkg/lifecycle/version_rules.go
Outdated
|
|
||
| // loadFromJSON will load the version rules from version_rules.json file at charts repository | ||
| func loadFromJSON(fs billy.Filesystem) (*VersionRules, error) { | ||
| vr := &VersionRules{ |
There was a problem hiding this comment.
This should be moved down to where it's used.
| minVersionStr := v.Rules[(branchVersionMinorSum(v.BranchVersion, -2))].Min | ||
| maxVersionStr := v.Rules[v.BranchVersion].Max | ||
|
|
||
| var err error |
There was a problem hiding this comment.
Error doesn't need and shouldn't be predeclared.
There was a problem hiding this comment.
In this case, I need it. Otherwise, I will get an error saying that the min and max variables are declared and not used.
@briandowns
pkg/lifecycle/version_rules.go
Outdated
|
|
||
| // CheckForRCVersion checks if the chart version contains the "-rc" string indicating a release candidate version. | ||
| func (v *VersionRules) CheckForRCVersion(chartVersion string) bool { | ||
| if strings.Contains(strings.ToLower(chartVersion), "-rc") { |
There was a problem hiding this comment.
This can be simplified to:
return strings.Contains(strings.ToLower(chartVersion), "-rc")
nicholasSUSE
force-pushed
the
refactoring-1
branch
from
29386bf to
3d2beb5
Compare
nicholasSUSE
force-pushed
the
refactoring-1
branch
from
a2684b8 to
c35c119
Compare
enforce-lifecyclecommand, since it was used only once with several limitations due to not all charts following the chart versioning standards.10...)