From b7da83afc34a780eb76659a887abb1a57a0306e9 Mon Sep 17 00:00:00 2001 From: Justin Pye Date: Wed, 16 Jul 2025 10:50:07 -0700 Subject: [PATCH 1/3] refactor: move FindImagesFromChart to helm package Move FindImagesFromChart from the images package to the helm package to avoid future circular dependency issues, since downstream packages like service use this function. This function is Helm-related, so it makes more sense for it to live in the helm package. The manifest command tests previously covered cmd.findAirbyteImages; these tests have now been moved to where the exported FindImagesFromChart function resides. --- internal/cmd/images/manifest_cmd.go | 124 +-------- internal/cmd/images/manifest_cmd_test.go | 114 -------- internal/helm/images.go | 108 +++++++ internal/helm/images_test.go | 340 +++++++++++++++++++++++ internal/service/install.go | 7 +- 5 files changed, 462 insertions(+), 231 deletions(-) delete mode 100644 internal/cmd/images/manifest_cmd_test.go create mode 100644 internal/helm/images.go create mode 100644 internal/helm/images_test.go diff --git a/internal/cmd/images/manifest_cmd.go b/internal/cmd/images/manifest_cmd.go index 96ecfb09..514df238 100644 --- a/internal/cmd/images/manifest_cmd.go +++ b/internal/cmd/images/manifest_cmd.go @@ -3,21 +3,12 @@ package images import ( "context" "fmt" - "slices" - "strings" + + goHelm "github.com/mittwald/go-helm-client" "github.com/airbytehq/abctl/internal/common" "github.com/airbytehq/abctl/internal/helm" "github.com/airbytehq/abctl/internal/trace" - helmlib "github.com/mittwald/go-helm-client" - "helm.sh/helm/v3/pkg/repo" - - appsv1 "k8s.io/api/apps/v1" - batchv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" - - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/kubectl/pkg/scheme" ) type ManifestCmd struct { @@ -30,7 +21,13 @@ func (c *ManifestCmd) Run(ctx context.Context) error { ctx, span := trace.NewSpan(ctx, "images manifest") defer span.End() - images, err := c.findAirbyteImages(ctx) + // TODO (bernielomax): Replace with service manager client factory. + client, err := goHelm.New(helm.ClientOptions(common.AirbyteNamespace)) + if err != nil { + return err + } + + images, err := c.findAirbyteImages(ctx, client) if err != nil { return err } @@ -42,7 +39,7 @@ func (c *ManifestCmd) Run(ctx context.Context) error { return nil } -func (c *ManifestCmd) findAirbyteImages(ctx context.Context) ([]string, error) { +func (c *ManifestCmd) findAirbyteImages(ctx context.Context, helmClient goHelm.Client) ([]string, error) { valuesYaml, err := helm.BuildAirbyteValues(ctx, helm.ValuesOpts{ ValuesFile: c.Values, }) @@ -51,104 +48,5 @@ func (c *ManifestCmd) findAirbyteImages(ctx context.Context) ([]string, error) { } airbyteChartLoc := helm.LocateLatestAirbyteChart(c.ChartVersion, c.Chart) - return FindImagesFromChart(valuesYaml, airbyteChartLoc, c.ChartVersion) -} - -func FindImagesFromChart(valuesYaml, chartName, chartVersion string) ([]string, error) { - - // sharing a helm client with the install code causes some weird issues, - // and templating the chart doesn't need details about the k8s provider, - // we create a throwaway helm client here. - client, err := helmlib.New(helm.ClientOptions(common.AirbyteNamespace)) - if err != nil { - return nil, err - } - - err = client.AddOrUpdateChartRepo(repo.Entry{ - Name: common.AirbyteRepoName, - URL: common.AirbyteRepoURL, - }) - if err != nil { - return nil, err - } - - bytes, err := client.TemplateChart(&helmlib.ChartSpec{ - ChartName: chartName, - GenerateName: true, - ValuesYaml: valuesYaml, - Version: chartVersion, - }, nil) - if err != nil { - return nil, err - } - - images := findAllImages(string(bytes)) - return images, nil -} - -// findAllImages walks through the Helm chart, looking for container images in k8s PodSpecs. -// It also looks for env vars in the airbyte-env config map that end with "_IMAGE". -// It returns a unique, sorted list of images found. -func findAllImages(chartYaml string) []string { - objs := decodeK8sResources(chartYaml) - imageSet := common.Set[string]{} - - for _, obj := range objs { - - var podSpec *corev1.PodSpec - switch z := obj.(type) { - case *corev1.ConfigMap: - if strings.HasSuffix(z.Name, "airbyte-env") { - for k, v := range z.Data { - if strings.HasSuffix(k, "_IMAGE") { - imageSet.Add(v) - } - } - } - continue - case *corev1.Pod: - podSpec = &z.Spec - case *batchv1.Job: - podSpec = &z.Spec.Template.Spec - case *appsv1.Deployment: - podSpec = &z.Spec.Template.Spec - case *appsv1.StatefulSet: - podSpec = &z.Spec.Template.Spec - default: - continue - } - - for _, c := range podSpec.InitContainers { - imageSet.Add(c.Image) - } - for _, c := range podSpec.Containers { - imageSet.Add(c.Image) - } - } - - var out []string - for _, k := range imageSet.Items() { - if k != "" { - out = append(out, k) - } - } - slices.Sort(out) - - return out -} - -func decodeK8sResources(renderedYaml string) []runtime.Object { - out := []runtime.Object{} - chunks := strings.Split(renderedYaml, "---") - for _, chunk := range chunks { - if len(chunk) == 0 { - continue - } - obj, _, err := scheme.Codecs.UniversalDeserializer().Decode([]byte(chunk), nil, nil) - if err != nil { - continue - } - out = append(out, obj) - } - return out + return helm.FindImagesFromChart(helmClient, valuesYaml, airbyteChartLoc, c.ChartVersion) } diff --git a/internal/cmd/images/manifest_cmd_test.go b/internal/cmd/images/manifest_cmd_test.go deleted file mode 100644 index 0df597f9..00000000 --- a/internal/cmd/images/manifest_cmd_test.go +++ /dev/null @@ -1,114 +0,0 @@ -package images - -import ( - "context" - "sort" - "testing" - - "github.com/google/go-cmp/cmp" -) - -func TestManifestCmd(t *testing.T) { - cmd := ManifestCmd{ - ChartVersion: "1.1.0", - } - actual, err := cmd.findAirbyteImages(context.Background()) - if err != nil { - t.Fatal(err) - } - expect := []string{ - "airbyte/bootloader:1.1.0", - "airbyte/connector-builder-server:1.1.0", - "airbyte/cron:1.1.0", - "airbyte/db:1.1.0", - "airbyte/mc", - "airbyte/server:1.1.0", - "airbyte/webapp:1.1.0", - "airbyte/worker:1.1.0", - "airbyte/workload-api-server:1.1.0", - "airbyte/workload-launcher:1.1.0", - "bitnami/kubectl:1.28.9", - "busybox", - "minio/minio:RELEASE.2023-11-20T22-40-07Z", - "temporalio/auto-setup:1.23.0", - } - compareList(t, expect, actual) -} - -func TestManifestCmd_Enterprise(t *testing.T) { - cmd := ManifestCmd{ - ChartVersion: "1.1.0", - Values: "testdata/enterprise.values.yaml", - } - actual, err := cmd.findAirbyteImages(context.Background()) - if err != nil { - t.Fatal(err) - } - expect := []string{ - "airbyte/bootloader:1.1.0", - "airbyte/connector-builder-server:1.1.0", - "airbyte/cron:1.1.0", - "airbyte/db:1.1.0", - "airbyte/keycloak-setup:1.1.0", - "airbyte/keycloak:1.1.0", - "airbyte/mc", - "airbyte/server:1.1.0", - "airbyte/webapp:1.1.0", - "airbyte/worker:1.1.0", - "airbyte/workload-api-server:1.1.0", - "airbyte/workload-launcher:1.1.0", - "bitnami/kubectl:1.28.9", - "busybox", - "curlimages/curl:8.1.1", - "minio/minio:RELEASE.2023-11-20T22-40-07Z", - "postgres:13-alpine", - "temporalio/auto-setup:1.23.0", - } - compareList(t, expect, actual) -} - -func TestManifestCmd_Nightly(t *testing.T) { - cmd := ManifestCmd{ - // This version includes chart fixes that expose images more consistently and completely. - ChartVersion: "1.1.0-nightly-1728428783-9025e1a46e", - Values: "testdata/enterprise.values.yaml", - } - actual, err := cmd.findAirbyteImages(context.Background()) - if err != nil { - t.Fatal(err) - } - expect := []string{ - "airbyte/bootloader:nightly-1728428783-9025e1a46e", - "airbyte/connector-builder-server:nightly-1728428783-9025e1a46e", - "airbyte/connector-sidecar:nightly-1728428783-9025e1a46e", - "airbyte/container-orchestrator:nightly-1728428783-9025e1a46e", - "airbyte/cron:nightly-1728428783-9025e1a46e", - "airbyte/db:nightly-1728428783-9025e1a46e", - "airbyte/keycloak-setup:nightly-1728428783-9025e1a46e", - "airbyte/keycloak:nightly-1728428783-9025e1a46e", - "airbyte/mc:latest", - "airbyte/server:nightly-1728428783-9025e1a46e", - "airbyte/webapp:nightly-1728428783-9025e1a46e", - "airbyte/worker:nightly-1728428783-9025e1a46e", - "airbyte/workload-api-server:nightly-1728428783-9025e1a46e", - "airbyte/workload-init-container:nightly-1728428783-9025e1a46e", - "airbyte/workload-launcher:nightly-1728428783-9025e1a46e", - "bitnami/kubectl:1.28.9", - "busybox:1.35", - "busybox:latest", - "curlimages/curl:8.1.1", - "minio/minio:RELEASE.2023-11-20T22-40-07Z", - "postgres:13-alpine", - "temporalio/auto-setup:1.23.0", - } - compareList(t, expect, actual) -} - -func compareList(t *testing.T, expect, actual []string) { - t.Helper() - sort.Strings(expect) - sort.Strings(actual) - if d := cmp.Diff(expect, actual); d != "" { - t.Error(d) - } -} diff --git a/internal/helm/images.go b/internal/helm/images.go new file mode 100644 index 00000000..ce474b43 --- /dev/null +++ b/internal/helm/images.go @@ -0,0 +1,108 @@ +package helm + +import ( + "context" + "slices" + "strings" + + goHelm "github.com/mittwald/go-helm-client" + "helm.sh/helm/v3/pkg/repo" + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/kubectl/pkg/scheme" + + "github.com/airbytehq/abctl/internal/common" +) + +func FindImagesFromChart(client goHelm.Client, valuesYaml, chartName, chartVersion string) ([]string, error) { + err := client.AddOrUpdateChartRepo(repo.Entry{ + Name: common.AirbyteRepoName, + URL: common.AirbyteRepoURL, + }) + if err != nil { + return nil, err + } + + rel, err := client.InstallChart(context.TODO(), &goHelm.ChartSpec{ + ChartName: chartName, + GenerateName: true, + ValuesYaml: valuesYaml, + Version: chartVersion, + DryRun: true, + }, nil) + if err != nil { + return nil, err + } + + images := findAllImages(rel.Manifest) + return images, nil +} + +// findAllImages walks through the Helm chart, looking for container images in k8s PodSpecs. +// It also looks for env vars in the airbyte-env config map that end with "_IMAGE". +// It returns a unique, sorted list of images found. +func findAllImages(chartYaml string) []string { + objs := decodeK8sResources(chartYaml) + imageSet := common.Set[string]{} + + for _, obj := range objs { + + var podSpec *corev1.PodSpec + switch z := obj.(type) { + case *corev1.ConfigMap: + if strings.HasSuffix(z.Name, "airbyte-env") { + for k, v := range z.Data { + if strings.HasSuffix(k, "_IMAGE") { + imageSet.Add(v) + } + } + } + continue + case *corev1.Pod: + podSpec = &z.Spec + case *batchv1.Job: + podSpec = &z.Spec.Template.Spec + case *appsv1.Deployment: + podSpec = &z.Spec.Template.Spec + case *appsv1.StatefulSet: + podSpec = &z.Spec.Template.Spec + default: + continue + } + + for _, c := range podSpec.InitContainers { + imageSet.Add(c.Image) + } + for _, c := range podSpec.Containers { + imageSet.Add(c.Image) + } + } + + var out []string + for _, k := range imageSet.Items() { + if k != "" { + out = append(out, k) + } + } + slices.Sort(out) + + return out +} + +func decodeK8sResources(renderedYaml string) []runtime.Object { + out := []runtime.Object{} + chunks := strings.Split(renderedYaml, "---") + for _, chunk := range chunks { + if len(chunk) == 0 { + continue + } + obj, _, err := scheme.Codecs.UniversalDeserializer().Decode([]byte(chunk), nil, nil) + if err != nil { + continue + } + out = append(out, obj) + } + return out +} diff --git a/internal/helm/images_test.go b/internal/helm/images_test.go new file mode 100644 index 00000000..cfe8091a --- /dev/null +++ b/internal/helm/images_test.go @@ -0,0 +1,340 @@ +package helm + +import ( + "os" + "sort" + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + "helm.sh/helm/v3/pkg/release" + + "github.com/airbytehq/abctl/internal/helm/mock" +) + +func TestFindImagesFromChart(t *testing.T) { + testCases := []struct { + name string + valuesPath string + chartName string + chartVersion string + expect []string + mockSetup func(client *mock.MockClient) + expectNilNoError bool + }{ + { + name: "enterprise-values-v1.1.0", + valuesPath: "../cmd/images/testdata/enterprise.values.yaml", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: []string{ + "airbyte/bootloader:1.1.0", + "airbyte/connector-builder-server:1.1.0", + "airbyte/cron:1.1.0", + "airbyte/db:1.1.0", + "airbyte/keycloak-setup:1.1.0", + "airbyte/keycloak:1.1.0", + "airbyte/mc", + "airbyte/server:1.1.0", + "airbyte/webapp:1.1.0", + "airbyte/worker:1.1.0", + "airbyte/workload-api-server:1.1.0", + "airbyte/workload-launcher:1.1.0", + "bitnami/kubectl:1.28.9", + "busybox", + "curlimages/curl:8.1.1", + "minio/minio:RELEASE.2023-11-20T22-40-07Z", + "postgres:13-alpine", + "temporalio/auto-setup:1.23.0", + }, + mockSetup: func(client *mock.MockClient) { + client.EXPECT().AddOrUpdateChartRepo(gomock.Any()).Return(nil) + client.EXPECT().InstallChart(gomock.Any(), gomock.Any(), gomock.Any()).Return(&release.Release{Manifest: sampleRenderedYaml}, nil) + }, + }, + { + name: "configmap-airbyte-env-image-keys", + valuesPath: "", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: []string{"img1:v1", "img2:v2", "shouldnotinclude"}, + mockSetup: func(client *mock.MockClient) { + client.EXPECT(). + AddOrUpdateChartRepo(gomock.Any()). + Return(nil) + client.EXPECT(). + InstallChart(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&release.Release{Manifest: configMapRenderedYaml}, nil) + }, + }, + { + name: "all-kinds-containers-initcontainers", + valuesPath: "", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: []string{"podimg", "jobimg", "deployimg", "statefulimg", "initimg"}, + mockSetup: func(client *mock.MockClient) { + client.EXPECT(). + AddOrUpdateChartRepo(gomock.Any()). + Return(nil) + client.EXPECT(). + InstallChart(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&release.Release{Manifest: allKindsRenderedYaml}, nil) + }, + }, + { + name: "addorupdatechartrepo-error", + valuesPath: "", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: nil, + mockSetup: func(client *mock.MockClient) { + client.EXPECT(). + AddOrUpdateChartRepo(gomock.Any()). + Return(assert.AnError) + }, + }, + { + name: "installchart-error", + valuesPath: "", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: nil, + mockSetup: func(client *mock.MockClient) { + client.EXPECT(). + AddOrUpdateChartRepo(gomock.Any()). + Return(nil) + client.EXPECT(). + InstallChart(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, assert.AnError) + }, + }, + { + name: "decodek8sresources-empty-invalid-chunks", + valuesPath: "", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: []string{"podimg"}, + mockSetup: func(client *mock.MockClient) { + client.EXPECT(). + AddOrUpdateChartRepo(gomock.Any()). + Return(nil) + client.EXPECT(). + InstallChart(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&release.Release{Manifest: decodeContinuesRenderedYaml}, nil) + }, + }, + { + name: "decodek8sresources-empty-chunk-only", + valuesPath: "", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: nil, + mockSetup: func(client *mock.MockClient) { + client.EXPECT(). + AddOrUpdateChartRepo(gomock.Any()). + Return(nil) + client.EXPECT(). + InstallChart(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&release.Release{Manifest: "---\n"}, nil) + }, + expectNilNoError: true, + }, + { + name: "decodek8sresources-unknown-kind-ignored", + valuesPath: "", + chartName: "airbyte/airbyte", + chartVersion: "1.1.0", + expect: nil, + mockSetup: func(client *mock.MockClient) { + client.EXPECT(). + AddOrUpdateChartRepo(gomock.Any()). + Return(nil) + client.EXPECT(). + InstallChart(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&release.Release{Manifest: unknownKindRenderedYaml}, nil) + }, + expectNilNoError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + client := mock.NewMockClient(ctrl) + if tc.mockSetup != nil { + tc.mockSetup(client) + } + + var valuesYaml []byte + var err error + if tc.valuesPath != "" { + valuesYaml, err = os.ReadFile(tc.valuesPath) + if err != nil { + t.Fatalf("failed to read values yaml: %v", err) + } + } + + images, err := FindImagesFromChart(client, string(valuesYaml), tc.chartName, tc.chartVersion) + if tc.expect == nil { + if tc.expectNilNoError { + assert.NoError(t, err) + assert.Nil(t, images) + } else { + assert.Error(t, err) + } + return + } + assert.NoError(t, err) + sort.Strings(tc.expect) + sort.Strings(images) + assert.Equal(t, tc.expect, images) + }) + } +} + +const configMapRenderedYaml = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-airbyte-env + namespace: default +data: + FOO_IMAGE: img1:v1 + BAR_IMAGE: img2:v2 + NOT_IMAGE: shouldnotinclude +` + +const allKindsRenderedYaml = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 +spec: + initContainers: + - name: init + image: initimg + containers: + - name: main + image: podimg +--- +apiVersion: batch/v1 +kind: Job +metadata: + name: job1 +spec: + template: + spec: + initContainers: + - name: init + image: initimg + containers: + - name: main + image: jobimg +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: deploy1 +spec: + template: + spec: + initContainers: + - name: init + image: initimg + containers: + - name: main + image: deployimg +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: stateful1 +spec: + template: + spec: + initContainers: + - name: init + image: initimg + containers: + - name: main + image: statefulimg +` + +const decodeContinuesRenderedYaml = ` +--- +# empty chunk above +invalid: [this is not valid yaml +--- +apiVersion: v1 +kind: Pod +metadata: + name: pod1 +spec: + containers: + - name: main + image: podimg +` + +const unknownKindRenderedYaml = ` +apiVersion: v1 +kind: Service +metadata: + name: svc1 +spec: + selector: + app: foo + ports: + - protocol: TCP + port: 80 + targetPort: 9376 +` + +// sampleRenderedYaml should be a string containing a rendered Helm chart YAML with all the images above. +// For brevity, you can use a minimal YAML or mock as needed for your actual test. +const sampleRenderedYaml = ` +apiVersion: v1 +kind: Pod +metadata: + name: test-pod +spec: + containers: + - name: bootloader + image: airbyte/bootloader:1.1.0 + - name: connector-builder-server + image: airbyte/connector-builder-server:1.1.0 + - name: cron + image: airbyte/cron:1.1.0 + - name: db + image: airbyte/db:1.1.0 + - name: keycloak-setup + image: airbyte/keycloak-setup:1.1.0 + - name: keycloak + image: airbyte/keycloak:1.1.0 + - name: mc + image: airbyte/mc + - name: server + image: airbyte/server:1.1.0 + - name: webapp + image: airbyte/webapp:1.1.0 + - name: worker + image: airbyte/worker:1.1.0 + - name: workload-api-server + image: airbyte/workload-api-server:1.1.0 + - name: workload-launcher + image: airbyte/workload-launcher:1.1.0 + - name: kubectl + image: bitnami/kubectl:1.28.9 + - name: busybox + image: busybox + - name: curl + image: curlimages/curl:8.1.1 + - name: minio + image: minio/minio:RELEASE.2023-11-20T22-40-07Z + - name: postgres + image: postgres:13-alpine + - name: temporalio + image: temporalio/auto-setup:1.23.0 +` diff --git a/internal/service/install.go b/internal/service/install.go index 7413e405..5a23075e 100644 --- a/internal/service/install.go +++ b/internal/service/install.go @@ -13,7 +13,6 @@ import ( "github.com/airbytehq/abctl/internal/abctl" "github.com/airbytehq/abctl/internal/airbyte" - "github.com/airbytehq/abctl/internal/cmd/images" "github.com/airbytehq/abctl/internal/common" "github.com/airbytehq/abctl/internal/docker" "github.com/airbytehq/abctl/internal/helm" @@ -92,7 +91,7 @@ func (m *Manager) persistentVolume(ctx context.Context, namespace, name string) path := filepath.Join(paths.Data, name) pterm.Debug.Println(fmt.Sprintf("Creating directory '%s'", path)) - if err := os.MkdirAll(path, 0766); err != nil { + if err := os.MkdirAll(path, 0o766); err != nil { pterm.Error.Println(fmt.Sprintf("Unable to create directory '%s'", name)) return fmt.Errorf("unable to create persistent volume '%s': %w", name, err) } @@ -114,7 +113,7 @@ func (m *Manager) persistentVolume(ctx context.Context, namespace, name string) // Due to the postgres uid/gid issue mentioned above, 0775 or 0755 would not allow the postgres image // access to the persisted volume directory. pterm.Debug.Println(fmt.Sprintf("Updating permissions for '%s'", path)) - if err := os.Chmod(path, 0777); err != nil { + if err := os.Chmod(path, 0o777); err != nil { pterm.Error.Println(fmt.Sprintf("Unable to set permissions for '%s'", path)) return fmt.Errorf("unable to set permissions for '%s': %w", path, err) } @@ -160,7 +159,7 @@ func (m *Manager) PrepImages(ctx context.Context, cluster k8s.Cluster, opts *Ins pterm.Info.Printfln("Patching image %s", image) } - manifest, err := images.FindImagesFromChart(opts.HelmValuesYaml, opts.AirbyteChartLoc, opts.HelmChartVersion) + manifest, err := helm.FindImagesFromChart(m.helm, opts.HelmValuesYaml, opts.AirbyteChartLoc, opts.HelmChartVersion) if err != nil { pterm.Debug.Printfln("error building image manifest: %s", err) return From f550de02a2488dec27109abaaff45275e2a188ce Mon Sep 17 00:00:00 2001 From: Justin Pye Date: Wed, 16 Jul 2025 10:52:44 -0700 Subject: [PATCH 2/3] refactor: move service manager factory code to service package Now that circular dependency issues have been resolved by moving FindImagesFromChart to the helm package, the service manager client factory can be shared more broadly. The manifest commands and other consumers can now use a single factory for dependency injection. Placing SvcMgrClientFactory in the service package centralizes client initialization logic and avoids future dependency issues. --- internal/cmd/cmd.go | 3 ++- internal/cmd/local/install.go | 2 +- internal/cmd/local/local.go | 28 ++-------------------------- internal/service/manager.go | 20 ++++++++++++++++++++ 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/internal/cmd/cmd.go b/internal/cmd/cmd.go index 80db3bf8..a58aa13a 100644 --- a/internal/cmd/cmd.go +++ b/internal/cmd/cmd.go @@ -7,6 +7,7 @@ import ( "github.com/airbytehq/abctl/internal/cmd/local" "github.com/airbytehq/abctl/internal/cmd/version" "github.com/airbytehq/abctl/internal/k8s" + "github.com/airbytehq/abctl/internal/service" "github.com/alecthomas/kong" "github.com/pterm/pterm" ) @@ -27,6 +28,6 @@ type Cmd struct { func (c *Cmd) BeforeApply(_ context.Context, kCtx *kong.Context) error { kCtx.BindTo(k8s.DefaultProvider, (*k8s.Provider)(nil)) - kCtx.BindTo(local.DefaultSvcMgrClientFactory, (*local.SvcMgrClientFactory)(nil)) + kCtx.BindTo(service.DefaultManagerClientFactory, (*service.ManagerClientFactory)(nil)) return nil } diff --git a/internal/cmd/local/install.go b/internal/cmd/local/install.go index 154aff13..7b6a69e0 100644 --- a/internal/cmd/local/install.go +++ b/internal/cmd/local/install.go @@ -34,7 +34,7 @@ type InstallCmd struct { } // Run executes the install command which creates the Kind cluster and installs the Airbyte service. -func (i *InstallCmd) Run(ctx context.Context, provider k8s.Provider, newSvcMgrClients SvcMgrClientFactory, telClient telemetry.Client) error { +func (i *InstallCmd) Run(ctx context.Context, provider k8s.Provider, newSvcMgrClients service.ManagerClientFactory, telClient telemetry.Client) error { ctx, span := trace.NewSpan(ctx, "local install") defer span.End() diff --git a/internal/cmd/local/local.go b/internal/cmd/local/local.go index f7229228..e8c8e7c1 100644 --- a/internal/cmd/local/local.go +++ b/internal/cmd/local/local.go @@ -6,15 +6,11 @@ import ( "io/fs" "os" - goHelm "github.com/mittwald/go-helm-client" "github.com/pterm/pterm" "github.com/airbytehq/abctl/internal/abctl" - "github.com/airbytehq/abctl/internal/common" - "github.com/airbytehq/abctl/internal/helm" "github.com/airbytehq/abctl/internal/k8s" "github.com/airbytehq/abctl/internal/paths" - "github.com/airbytehq/abctl/internal/service" ) type Cmd struct { @@ -25,10 +21,6 @@ type Cmd struct { Uninstall UninstallCmd `cmd:"" help:"Uninstall local Airbyte."` } -// SvcMgrClientFactory creates and returns the Kubernetes and Helm clients -// needed by the service manager. -type SvcMgrClientFactory func(kubeConfig, kubeContext string) (k8s.Client, goHelm.Client, error) - func (c *Cmd) BeforeApply() error { if _, envVarDNT := os.LookupEnv("DO_NOT_TRACK"); envVarDNT { pterm.Info.Println("Telemetry collection disabled (DO_NOT_TRACK)") @@ -70,30 +62,14 @@ func checkAirbyteDir() error { return errors.New(paths.Airbyte + " is not a directory") } - if fileInfo.Mode().Perm() >= 0744 { + if fileInfo.Mode().Perm() >= 0o744 { // directory has minimal permissions return nil } - if err := os.Chmod(paths.Airbyte, 0744); err != nil { + if err := os.Chmod(paths.Airbyte, 0o744); err != nil { return fmt.Errorf("unable to change permissions of '%s': %w", paths.Airbyte, err) } return nil } - -// DefaultSvcMgrClientFactory initializes and returns the default Kubernetes -// and Helm clients for the service manager. -func DefaultSvcMgrClientFactory(kubeConfig, kubeContext string) (k8s.Client, goHelm.Client, error) { - kubeClient, err := service.DefaultK8s(kubeConfig, kubeContext) - if err != nil { - return nil, nil, fmt.Errorf("failed to initialize the kubernetes client: %w", err) - } - - helmClient, err := helm.New(kubeConfig, kubeContext, common.AirbyteNamespace) - if err != nil { - return nil, nil, fmt.Errorf("failed to initialize the helm client: %w", err) - } - - return kubeClient, helmClient, nil -} diff --git a/internal/service/manager.go b/internal/service/manager.go index c1566542..1db5983e 100644 --- a/internal/service/manager.go +++ b/internal/service/manager.go @@ -28,6 +28,10 @@ import ( "k8s.io/client-go/tools/clientcmd" ) +// ManagerClientFactory creates and returns the Kubernetes and Helm clients +// needed by the service manager. +type ManagerClientFactory func(kubeConfig, kubeContext string) (k8s.Client, goHelm.Client, error) + type HTTPClient interface { Do(req *http.Request) (*http.Response, error) } @@ -236,3 +240,19 @@ func EnablePsql17() (bool, error) { return false, nil } + +// DefaultManagerClientFactory initializes and returns the default Kubernetes +// and Helm clients for the service manager. +func DefaultManagerClientFactory(kubeConfig, kubeContext string) (k8s.Client, goHelm.Client, error) { + kubeClient, err := DefaultK8s(kubeConfig, kubeContext) + if err != nil { + return nil, nil, fmt.Errorf("failed to initialize the kubernetes client: %w", err) + } + + helmClient, err := helm.New(kubeConfig, kubeContext, common.AirbyteNamespace) + if err != nil { + return nil, nil, fmt.Errorf("failed to initialize the helm client: %w", err) + } + + return kubeClient, helmClient, nil +} From 5330e8077666844e50bef530f5740dbe02dde7fe Mon Sep 17 00:00:00 2001 From: Justin Pye Date: Wed, 16 Jul 2025 14:11:33 -0700 Subject: [PATCH 3/3] refactor: images manifest command uses service manager factory --- internal/cmd/images/manifest_cmd.go | 11 +++++++---- internal/common/const.go | 1 + internal/k8s/provider.go | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/cmd/images/manifest_cmd.go b/internal/cmd/images/manifest_cmd.go index 514df238..7223dc68 100644 --- a/internal/cmd/images/manifest_cmd.go +++ b/internal/cmd/images/manifest_cmd.go @@ -8,6 +8,8 @@ import ( "github.com/airbytehq/abctl/internal/common" "github.com/airbytehq/abctl/internal/helm" + "github.com/airbytehq/abctl/internal/paths" + "github.com/airbytehq/abctl/internal/service" "github.com/airbytehq/abctl/internal/trace" ) @@ -17,17 +19,18 @@ type ManifestCmd struct { Values string `type:"existingfile" help:"An Airbyte helm chart values file to configure helm."` } -func (c *ManifestCmd) Run(ctx context.Context) error { +func (c *ManifestCmd) Run(ctx context.Context, newSvcMgrClients service.ManagerClientFactory) error { ctx, span := trace.NewSpan(ctx, "images manifest") defer span.End() - // TODO (bernielomax): Replace with service manager client factory. - client, err := goHelm.New(helm.ClientOptions(common.AirbyteNamespace)) + // Load the required service manager clients. We only need the Helm client + // for image manifest operations. + _, helmClient, err := newSvcMgrClients(paths.Kubeconfig, common.AirbyteKubeContext) if err != nil { return err } - images, err := c.findAirbyteImages(ctx, client) + images, err := c.findAirbyteImages(ctx, helmClient) if err != nil { return err } diff --git a/internal/common/const.go b/internal/common/const.go index c4f34a3e..f195593f 100644 --- a/internal/common/const.go +++ b/internal/common/const.go @@ -6,6 +6,7 @@ const ( AirbyteChartRelease = "airbyte-abctl" AirbyteIngress = "ingress-abctl" AirbyteNamespace = "airbyte-abctl" + AirbyteKubeContext = "kind-airbyte-abctl" AirbyteRepoName = "airbyte" AirbyteRepoURL = "https://airbytehq.github.io/helm-charts" NginxChartName = "nginx/ingress-nginx" diff --git a/internal/k8s/provider.go b/internal/k8s/provider.go index cfb05aa4..d3076d65 100644 --- a/internal/k8s/provider.go +++ b/internal/k8s/provider.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" + "github.com/airbytehq/abctl/internal/common" "github.com/airbytehq/abctl/internal/paths" "github.com/airbytehq/abctl/internal/trace" "github.com/pterm/pterm" @@ -99,7 +100,7 @@ var ( DefaultProvider = Provider{ Name: Kind, ClusterName: "airbyte-abctl", - Context: "kind-airbyte-abctl", + Context: common.AirbyteKubeContext, Kubeconfig: paths.Kubeconfig, }