feat: add images manifest command #133

abuchanan-airbyte · 2024-10-09T19:22:55Z

https://github.com/airbytehq/airbyte-internal-issues/issues/10018

This adds abctl images manifest which prints a list of container images used by the Airbyte platform.

abctl git:(abuch/images-cmd) ./abctl images manifest
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

It takes --chart, --chart-version, and --values flags (similar to local install) which can affect the outcome. For example, if you're values.yaml configures the enterprise edition, the image manifest will include keycloak and any other enterprise-specific images.

I needed to refactor some code in order to share Helm functions between the install and images commands. In particular, I've pulled a lot of Helm values building up into BuildAirbyteValues and InstallCmd.InstallOpts(). I also pulled some shared contants into a new common package.

I deduped the mockTelemetryClient into telemetry.MockClient and simplified it a bit, so the test code using this is simpler. I refactored telemetry.Client.User() UUID to telemetry.Client.User() string (segment still uses a UUID behind the scenes of course, it's just not exposed in the Go interface anymore).

Testing the manifest command involves pulling down actual charts (1.1.0 and a recent nightly version). This is somewhat lame, but without this the test wouldn't actually test much.

abuchanan-airbyte · 2024-10-09T19:24:39Z

internal/cmd/cmd.go

 	Version version.Cmd `cmd:"" help:"Display version information."`
 	Verbose verbose     `short:"v" help:"Enable verbose output."`
 }

 func (c *Cmd) BeforeApply(ctx *kong.Context) error {
-	if _, envVarDNT := os.LookupEnv("DO_NOT_TRACK"); envVarDNT {
-		pterm.Info.Println("Telemetry collection disabled (DO_NOT_TRACK)")
-	}


This moved to the local command, so that it doesn't print at the top of commands like images manifest which aren't related to telemetry.

abuchanan-airbyte · 2024-10-09T19:33:17Z

internal/cmd/local/local/install_test.go

 		t.Fatal(err)
 	}
 }

-func TestCommand_Install_HelmValues(t *testing.T) {


After refactoring the building of InstallOpts, this test became a dupe of TestCommand_Install because Install() doesn't build any values YAML now, it receives the fully formed values YAML from the caller.

abuchanan-airbyte · 2024-10-09T19:35:02Z

internal/cmd/local/local_install.go

@@ -148,6 +170,10 @@ func (i *InstallCmd) Run(ctx context.Context, provider k8s.Provider, telClient t
 }

 func parseVolumeMounts(specs []string) ([]k8s.ExtraVolumeMount, error) {
+	if len(specs) == 0 {
+		return nil, nil
+	}


This allows tests to pass easily with zero-value struct fields, because the code below uses make which doesn't return a nil slice when there are no volume mounts.

abuchanan-airbyte · 2024-10-09T19:35:40Z

internal/cmd/local/local_test.go

+		kong.Description("Airbyte's command line tool for managing a local Airbyte installation."),
+		kong.UsageOnError(),
+	)
+	_, err := k.Parse([]string{"--values", "/testdata/thisfiledoesnotexist"})


The code is relying on Kong's existingfile validation now, instead of having a manual check (for better or worse)

abuchanan-airbyte · 2024-10-09T19:36:31Z

internal/telemetry/client.go

@@ -43,7 +42,7 @@ type Client interface {
 	// Attr should be called to add additional attributes to this activity.
 	Attr(key, val string)
 	// User returns the user identifier being used by this client
-	User() uuid.UUID
+	User() string


As noted in the PR description, having User() return a string simplifies some test code slightly.

Can you expand on exactly how this simplifies the test code?

Tests don't need to generate and set a UUID on the mock. The tests can expect a consistent "test-user", and for the install test, that meant I could move the expected output to a testdata yaml file

bgroff · 2024-10-09T19:47:16Z

go.mod

@@ -14,13 +14,14 @@ require (
 	github.com/opencontainers/image-spec v1.1.0-rc6
 	github.com/pbnjay/memory v0.0.0-20210728143218-7b4eea64cf58
 	github.com/pterm/pterm v0.12.79
+	github.com/stretchr/testify v1.8.4


Can we communicate this to the rest of the team.

@colesnodgrass @perangel I used testify/assert to assert a list of values.

I've been avoiding using the testify package as I don't think it's necessary for most situations, and it feels a little overkill to bring it in for one function.

Most of the code in abctl uses the cmp package and we can easily use that package for verifying the two lists match

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) } }

Is it really causing any trouble? It's handy and popular.

While I'm personally not a huge fan of testify (I also haven't looked at in in years, so maybe it is worth taking another look with older eyes). If everyone else wants to start using it, I'll be fine with that. I care mostly about simplicity and consistency.

go.mod

colesnodgrass

Do we foresee adding other sub-commands under the images command? In other words, do we need two levels here (abctl images manifest) or should we collapse this into a single new images command (abctl images)?

colesnodgrass · 2024-10-10T20:58:25Z

internal/cmd/images/manifest_cmd.go

+
+type ManifestCmd struct {
+	Chart        string `help:"Path to chart." xor:"chartver"`
+	ChartVersion string `help:"Version to install." xor:"chartver"`


The help comment seems incorrect, as this isn't being installed by this command.

colesnodgrass · 2024-10-10T21:02:27Z

internal/cmd/images/manifest_cmd.go

+// It returns a unique, sorted list of images found.
+func findAllImages(chartYaml string) []string {
+	objs := decodeK8sResources(chartYaml)
+	imageSet := map[string]bool{}


Could use map[string]struct{}{} as the bool value isn't being used for anything.

I prefer bool because it's shorter

I get the shorter argument. In this specific case I'm ok with the bool approach. However, in general, for faux-set implementations I still prefer the struct{} approach as there is never a question of "What if I set this bool value to false, does the code behave differently?"

Good point. I do wish Go would add a container/set package. I wrote a small one in the code to replace the map[string]bool

colesnodgrass · 2024-10-10T21:13:19Z

internal/telemetry/client.go

@@ -43,7 +42,7 @@ type Client interface {
 	// Attr should be called to add additional attributes to this activity.
 	Attr(key, val string)
 	// User returns the user identifier being used by this client
-	User() uuid.UUID
+	User() string


Can you expand on exactly how this simplifies the test code?

perangel · 2024-10-11T12:38:59Z

Do we foresee adding other sub-commands under the images command? In other words, do we need two levels here (abctl images manifest) or should we collapse this into a single new images command (abctl images)?

In can imagine a command that does the copying of images from our repo to the customers private repository.

colesnodgrass · 2024-10-11T16:24:47Z

@perangel

In can imagine a command that does the copying of images from our repo to the customers private repository.

This makes sense. Should this be abctl image ... or abctl images ...?

feat: add images manifest command

59466fd

abuchanan-airbyte requested a review from a team as a code owner October 9, 2024 19:22

abuchanan-airbyte commented Oct 9, 2024

View reviewed changes

abuchanan-airbyte added 2 commits October 9, 2024 12:26

test with simpler helm client

72610dc

go fmt

ae7f297

abuchanan-airbyte commented Oct 9, 2024

View reviewed changes

fix test

5c665e0

bgroff approved these changes Oct 9, 2024

View reviewed changes

colesnodgrass reviewed Oct 10, 2024

View reviewed changes

pr review cleanup

0b4700b

change the image set data structure

045f363

abuchanan-airbyte merged commit 080307b into main Oct 14, 2024
2 checks passed

abuchanan-airbyte deleted the abuch/images-cmd branch October 14, 2024 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add images manifest command #133

feat: add images manifest command #133

abuchanan-airbyte commented Oct 9, 2024 •

edited

Loading

abuchanan-airbyte Oct 9, 2024

abuchanan-airbyte Oct 9, 2024

abuchanan-airbyte Oct 9, 2024

abuchanan-airbyte Oct 9, 2024

abuchanan-airbyte Oct 9, 2024

colesnodgrass Oct 10, 2024

abuchanan-airbyte Oct 11, 2024

bgroff Oct 9, 2024

abuchanan-airbyte Oct 9, 2024

colesnodgrass Oct 10, 2024

abuchanan-airbyte Oct 11, 2024

colesnodgrass Oct 14, 2024

colesnodgrass left a comment

colesnodgrass Oct 10, 2024

colesnodgrass Oct 10, 2024

abuchanan-airbyte Oct 11, 2024

colesnodgrass Oct 11, 2024

abuchanan-airbyte Oct 11, 2024

colesnodgrass Oct 10, 2024

perangel commented Oct 11, 2024

colesnodgrass commented Oct 11, 2024

feat: add images manifest command #133

feat: add images manifest command #133

Conversation

abuchanan-airbyte commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesnodgrass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perangel commented Oct 11, 2024

colesnodgrass commented Oct 11, 2024

abuchanan-airbyte commented Oct 9, 2024 •

edited

Loading