chore(vclusterctl): check for pre 0.20 values #1738
Conversation
✅ Deploy Preview for vcluster-docs canceled.Built without sensitive environment variables
|
johannesfrey
force-pushed
the
check-pre-0.20-values-vclusterctl-3
branch
22 times, most recently
from
5ca8801
to
f0c58c4
Compare
johannesfrey
force-pushed
the
check-pre-0.20-values-vclusterctl-3
branch
from
f0c58c4
to
2960e22
Compare
pkg/cli/create_helm.go
Outdated
| } | ||
|
|
||
| command := fmt.Sprintf("vcluster convert config --distro %s - <<EOF\n%s\nEOF", currentDistro, string(currentValues)) |
There was a problem hiding this comment.
This will print a big chunk, can we not do something like:
helm get values ... | vcluster convert config ...
There was a problem hiding this comment.
Sure. helm get values will retrieve the latest revision right? I tested locally and it looks like it 🤔 And should we do a helm get values -a? Otherwise, if you happened to install it with default config via helm helm get values will return null.
Update: I made the helm command conditionally.
pkg/cli/create_helm.go
Outdated
| if err != nil { | ||
| if errors.Is(err, config.ErrInvalidConfig) { | ||
| cmd.log.Infof("If you are using the old values format, consider using %q to convert it to the new v0.20 format", "vcluster convert config") | ||
| if !errors.Is(err, config.ErrInvalidConfig) { | ||
| return err | ||
| } | ||
| return err | ||
| // TODO Delete after vCluster 0.19.x resp. the old config format is out of support. | ||
| // We cannot discriminate between k0s/k3s and eks/k8s. So we cannot prompt the actual values to convert, as this would cause false positives, | ||
| // because users are free to e.g. pass a k0s values file to a currently running k3s virtual cluster. | ||
| if isLegacyConfig(data) { | ||
| return fmt.Errorf("it appears you are using a vCluster configuration using pre-v0.20 formatting. Please run %q to convert the values to the latest format", "vcluster convert config") | ||
| } | ||
| // TODO end | ||
| } |
There was a problem hiding this comment.
I would maybe structure the if differently here
if errors.Is(err, config.ErrInvalidConfig) {
if isLegacyConfig(data){
return fmt.Errorf("it appears you are using a vCluster configuration using pre-v0.20 formatting. Please run %q to convert the values to the latest format", "vcluster convert config")
}
} else if err != nil {
return err
}that is simpler to follow imho
There was a problem hiding this comment.
btw don't we want to return the error if it's ErrInvalidConfig but not isLegacyConfig ?
There was a problem hiding this comment.
Update: The ErrInvalidConfig did not provide any benefit. It was actually a left-over from when the config unmarshaling looked different. So I changed it to a simple err check.
pkg/cli/create_helm.go
Outdated
| values, err := yaml.Marshal(helmChart.Chart.Values) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return values, nil |
There was a problem hiding this comment.
here you can just do
return yaml.Marshal(helmChart.Chart.Values)There was a problem hiding this comment.
When going with Fabian's suggestion this func will be removed.
pkg/cli/create_helm.go
Outdated
| releaseRaw, err := io.ReadAll(gz) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| helmChart := struct { | ||
| Chart struct { | ||
| Values map[string]interface{} `json:"values"` | ||
| } `json:"chart"` | ||
| }{} | ||
|
|
||
| // TODO end | ||
| if err := yaml.Unmarshal(releaseRaw, &helmChart); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
can we use a decoder here instead of reading everything into memory or not?
There was a problem hiding this comment.
When going with Fabian's suggestion this func will be removed.
johannesfrey
force-pushed
the
check-pre-0.20-values-vclusterctl-3
branch
from
a3f09bc
to
3cf3b82
Compare
johannesfrey
force-pushed
the
check-pre-0.20-values-vclusterctl-3
branch
from
3cf3b82
to
6ad1417
Compare
What issue type does this pull request address? (keep at least one, remove the others)
/kind enhancement
What does this pull request do? Which issues does it resolve? (use
resolves #<issue_number>if possible)Resolves ENG-3573
Please provide a short message that should be published in the vcluster release notes
Provide checks that abort "vcluster create" when using pre-v0.20 formatting in values.yaml.
What else do we need to know?