Release charts update cmd #464
Conversation
nicholasSUSE
force-pushed
the
release-charts-update-cmd
branch
from
b2a785f to
ea03c74
Compare
nicholasSUSE
requested review from
briandowns,
tashima42,
brooksn and
rafaelbreno
nicholasSUSE
force-pushed
the
release-charts-update-cmd
branch
from
e253f9a to
4d16e09
Compare
nicholasSUSE
force-pushed
the
release-charts-update-cmd
branch
from
d227123 to
24523a8
Compare
cmd/release/cmd/update.go
Outdated
|
|
||
| var branch, chart, version string | ||
| var found bool | ||
| var err error |
There was a problem hiding this comment.
This can be a problem when an error is checked and the action is not to return which leaves this value potentially untouched on a subsequent check causing issues. Let's not define a catch-all error value here.
release/charts/arguments.go
Outdated
| @@ -0,0 +1,132 @@ | |||
| package charts | |||
There was a problem hiding this comment.
What's the context for the name arguments? If it's related to the command(s) being ran, that's an implementation leak and should be renamed to represent the business logic and now how the logic is used.
There was a problem hiding this comment.
Implemented a fix from what I could at understand at:
"changing naming strategy from arguments.go -> chart_management.go"
Commit.
release/charts/arguments.go
Outdated
|
|
||
| // BranchArgs will return the list of available branch version lines | ||
| func BranchArgs() []string { | ||
| return []string{"2.9", "2.8"} |
There was a problem hiding this comment.
Can this be part of configuration to avoid code changes when things change?
cmd/release/cmd/update.go
Outdated
| @@ -3,7 +3,10 @@ package cmd | |||
| import ( | |||
| "context" | |||
| "errors" | |||
| "fmt" | |||
| "log" | |||
There was a problem hiding this comment.
Let's remove this in favor of regular CLI outut. We don't have anything consuming these logs and they're created at the time of execution so their timestamp values don't give us any extra value.
Issue:
#446
Solution:
After implementing: rancher/charts-build-scripts#145
This is the integration between:
charts-build-scriptscommandreleaseecm-distro-toolscommandrelease update chartsResult example:
Extra
Implemented auto-completion for the 3 arguments on the command:
<branch>has 2 options:2.8 and 2.9<chart>will auto-complete all available charts in the repo.<version>will check the state.json file generated from the list command, look in real-time for available release versions, and auto-complete that.This extra is very very useful!