Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure --values settings override internal defaults #64

Merged
merged 5 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 33 additions & 14 deletions internal/cmd/local/local/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/airbytehq/abctl/internal/cmd/local/helm"
"github.com/airbytehq/abctl/internal/cmd/local/migrate"
"github.com/airbytehq/abctl/internal/cmd/local/paths"
"github.com/airbytehq/abctl/internal/maps"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/release"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -306,15 +307,6 @@ func (c *Command) persistentVolumeClaim(ctx context.Context, namespace, name, vo

// Install handles the installation of Airbyte
func (c *Command) Install(ctx context.Context, opts InstallOpts) error {
var vals string
if opts.ValuesFile != "" {
raw, err := os.ReadFile(opts.ValuesFile)
if err != nil {
return fmt.Errorf("unable to read values file '%s': %w", opts.ValuesFile, err)
}
vals = string(raw)
}

go c.watchEvents(ctx)

if !c.k8s.NamespaceExists(ctx, airbyteNamespace) {
Expand Down Expand Up @@ -358,9 +350,9 @@ func (c *Command) Install(ctx context.Context, opts InstallOpts) error {
}

airbyteValues := []string{
fmt.Sprintf("global.env_vars.AIRBYTE_INSTALLATION_ID=%s", telUser),
fmt.Sprintf("global.jobs.resources.limits.cpu=3"),
fmt.Sprintf("global.jobs.resources.limits.memory=4Gi"),
"global.env_vars.AIRBYTE_INSTALLATION_ID=" + telUser,
"global.jobs.resources.limits.cpu=3",
"global.jobs.resources.limits.memory=4Gi",
}

if opts.dockerAuth() {
Expand Down Expand Up @@ -396,6 +388,11 @@ func (c *Command) Install(ctx context.Context, opts InstallOpts) error {
pterm.Success.Println(fmt.Sprintf("Secret from '%s' created or updated", secretFile))
}

valuesYAML, err := mergeValuesWithValuesYAML(airbyteValues, opts.ValuesFile)
if err != nil {
return fmt.Errorf("unable to merge values with values file '%s': %w", opts.ValuesFile, err)
}

if err := c.handleChart(ctx, chartRequest{
name: "airbyte",
repoName: airbyteRepoName,
Expand All @@ -404,8 +401,7 @@ func (c *Command) Install(ctx context.Context, opts InstallOpts) error {
chartRelease: airbyteChartRelease,
chartVersion: opts.HelmChartVersion,
namespace: airbyteNamespace,
values: airbyteValues,
valuesYAML: vals,
valuesYAML: valuesYAML,
}); err != nil {
return fmt.Errorf("unable to install airbyte chart: %w", err)
}
Expand Down Expand Up @@ -871,3 +867,26 @@ func checkHelmReleaseShouldInstall(helm helm.Client, chart *chart.Chart, release

return false
}

// mergeValuesWithValuesYAML ensures that the values defined within this code have a lower
// priority than any values defined in a values.yaml file.
// By default, the helm-client we're using reversed this priority, putting the values
// defined in this code at a higher priority than the values defined in the values.yaml file.
// This function returns a string representation of the value.yaml file after all
// values provided were potentially overridden by the valuesYML file.
func mergeValuesWithValuesYAML(values []string, valuesYAML string) (string, error) {
a := maps.FromSlice(values)
b, err := maps.FromYAMLFile(valuesYAML)
if err != nil {
return "", fmt.Errorf("unable to read values from yaml file '%s': %w", valuesYAML, err)
}
maps.Merge(a, b)

res, err := maps.ToYAML(a)
if err != nil {
return "", fmt.Errorf("unable to merge values from yaml file '%s': %w", valuesYAML, err)
}

return res, nil

}
35 changes: 23 additions & 12 deletions internal/cmd/local/local/cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,15 @@ func TestCommand_Install(t *testing.T) {
CreateNamespace: true,
Wait: true,
Timeout: 30 * time.Minute,
ValuesOptions: values.Options{Values: []string{
"global.env_vars.AIRBYTE_INSTALLATION_ID=" + userID.String(),
"global.jobs.resources.limits.cpu=3",
"global.jobs.resources.limits.memory=4Gi",
}},
ValuesYaml: `global:
env_vars:
AIRBYTE_INSTALLATION_ID: ` + userID.String() + `
jobs:
resources:
limits:
cpu: "3"
memory: 4Gi
`,
},
release: release.Release{
Chart: &chart.Chart{Metadata: &chart.Metadata{Version: "1.2.3.4"}},
Expand Down Expand Up @@ -205,12 +209,16 @@ func TestCommand_Install_ValuesFile(t *testing.T) {
CreateNamespace: true,
Wait: true,
Timeout: 30 * time.Minute,
ValuesOptions: values.Options{Values: []string{
"global.env_vars.AIRBYTE_INSTALLATION_ID=" + userID.String(),
"global.jobs.resources.limits.cpu=3",
"global.jobs.resources.limits.memory=4Gi",
}},
ValuesYaml: "global:\n edition: \"test\"\n",
ValuesYaml: `global:
edition: test
env_vars:
AIRBYTE_INSTALLATION_ID: ` + userID.String() + `
jobs:
resources:
limits:
cpu: "3"
memory: 4Gi
`,
},
release: release.Release{
Chart: &chart.Chart{Metadata: &chart.Metadata{Version: "1.2.3.4"}},
Expand Down Expand Up @@ -356,7 +364,7 @@ func TestCommand_Install_InvalidValuesFile(t *testing.T) {
if err == nil {
t.Fatal("expecting an error, received none")
}
if !strings.Contains(err.Error(), fmt.Sprintf("unable to read values file '%s'", valuesFile)) {
if !strings.Contains(err.Error(), fmt.Sprintf("unable to read values from yaml file '%s'", valuesFile)) {
t.Error("unexpected error:", err)
}

Expand Down Expand Up @@ -560,6 +568,9 @@ func (m *mockTelemetryClient) Attr(key, val string) {
}

func (m *mockTelemetryClient) User() uuid.UUID {
if m.user == nil {
return uuid.Nil
}
return m.user()
}

Expand Down
103 changes: 103 additions & 0 deletions internal/maps/maps.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
package maps

import (
"fmt"
"gopkg.in/yaml.v3"
"os"
"strings"
)

// FromSlice converts a slice of dot-delimited string values into a map[string]any.
// For example:
// "a.b.c=123","a.b.d=124" would return { "a": { "b": { "c": 123, "d": 124 } } }
func FromSlice(slice []string) map[string]any {
m := map[string]any{}

for _, s := range slice {
// s has the format of:
// a.b.c=xyz
parts := strings.SplitN(s, "=", 2)
Copy link
Collaborator

@perangel perangel Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to cause any issues if the value contains = as in a base64 encoded string (e.g. DEADBEEF==)? Would you mind adding a test case for that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is why I am using SplitN with 2, so it will only split on the first =.

If the key was base64 encoded, then that would be a problem.

// a.b.c becomes [a, b, c]
keys := strings.Split(parts[0], ".")
// xyz
value := parts[1]

// pointer to the root of the map,
// as this map will contain nested maps, this pointer will be
// updated to point to the root of the nested maps as it iterates
// through the for loop
p := m
for i, k := range keys {
// last key, put the value into the map
if i == len(keys)-1 {
p[k] = value
continue
}
// if the nested map doesn't exist, create it
if _, ok := p[k]; !ok {
p[k] = map[string]any{}
}
// cast the key to a map[string]any
// and update the pointer to point to it
p = p[k].(map[string]any)
}
}

return m
}

// FromYAMLFile converts a yaml file into a map[string]any.
func FromYAMLFile(path string) (map[string]any, error) {
if path == "" {
return map[string]any{}, nil
}

raw, err := os.ReadFile(path)
if err != nil {
return nil, fmt.Errorf("failed to read file %s: %w", path, err)
}
var m map[string]any
if err := yaml.Unmarshal(raw, &m); err != nil {
return nil, fmt.Errorf("failed to unmarshal file %s: %w", path, err)
}
// ensure we don't return `nil, nil`
if m == nil {
return map[string]any{}, nil
}
return m, nil
}

// ToYAML converts the m map into a yaml string.
// E.g. map[string]any{"a" : 1, "b", 2} becomes
// a: 1
// b: 2
func ToYAML(m map[string]any) (string, error) {
raw, err := yaml.Marshal(m)
if err != nil {
return "", fmt.Errorf("failed to marshal map: %w", err)
}
return string(raw), nil
}

// Merge merges the override map into the base map.
// Modifying the base map in place.
func Merge(base, override map[string]any) {
for k, overrideVal := range override {
if baseVal, ok := base[k]; ok {
// both maps have this key
baseChild, baseChildIsMap := baseVal.(map[string]any)
overrideChild, overrideChildIsMap := overrideVal.(map[string]any)

if baseChildIsMap && overrideChildIsMap {
// both values are maps, recurse
Merge(baseChild, overrideChild)
} else {
// override base with override
base[k] = overrideVal
}
} else {
// only override has this key
base[k] = overrideVal
}
}
}
Loading