CLOUDP-289219: Create CLI commands for manipulating stream processors #3496

auddin431 · 2024-12-19T22:06:07Z

Proposed changes

This work adds the noun processors to the top level noun streams so that stream processors can be interacted with through the CLI. The processors noun has the following verbs:

create
delete
update
list
describe
start
stop

Jira ticket: CLOUDP-289219

Checklist

I have signed the MongoDB CLA
I have added tests that prove my fix is effective or that my feature works
I have added any necessary documentation in document requirements section listed in CONTRIBUTING.md (if appropriate)
I have addressed the @mongodb/docs-cloud-team comments (if appropriate)
I have updated test/README.md (if an e2e test has been added)
I have run make fmt and formatted my code

tcannon91

Next time, it might be a little easier to review if we break down the commits into each command or each command into their own PR :)

Dropping comments because I think I have gone through all of the create command. I haven't looked but I will try not to leave the same comments on the other commands if they have the same issues. I will continue looking at the other commands and try to only leave net new stuff

tcannon91 · 2024-12-20T19:43:22Z

internal/store/streams.go

+	ListProcessors(*atlasv2.ListStreamProcessorsApiParams) (*atlasv2.PaginatedApiStreamsStreamProcessorWithStats, error)
+}
+
+type ProcessorDescriber interface {
+	StreamProcessor(*atlasv2.GetStreamProcessorApiParams) (*atlasv2.StreamsProcessorWithStats, error)
+}
+
+type ProcessorStarter interface {
+	StartStreamProcessor(*atlasv2.StartStreamProcessorApiParams) error
+}
+
+type ProcessorStopper interface {
+	StopStreamProcessor(*atlasv2.StopStreamProcessorApiParams) error
+}
+
+type ProcessorDeleter interface {
+	DeleteStreamProcessor(string, string, string) error
+}
+
+type ProcessorCreator interface {
+	CreateStreamProcessor(*atlasv2.CreateStreamProcessorApiParams) (*atlasv2.StreamsProcessor, error)


Can you name the arguments to each of these functions?

For example, it is hard to know what each of the parameters are/should be for DeleteStreamProcessor

tcannon91 · 2024-12-20T19:46:03Z

internal/store/streams.go

+}
+
+// DeleteStreamProcessor deletes the specified Stream Processor.
+func (s *Store) DeleteStreamProcessor(projectID, tenantName, processorName string) error {


It is interesting that the other functions here all take a request and pull what they need while this method takes each value as a string. I have a personal preference towards this approach (explicitly calling for what you need rather than taking a whole request object), but more important than one direction vs the other I think we should be consistent.

Pick one approach and stick to it for the implementations unless there is something that makes this method especially unique.

tcannon91 · 2024-12-20T20:17:29Z

docs/command/atlas-streams-processors-create.txt

+   * - -o, --output
+     - string
+     - false
+     - Output format. Valid values are json, json-path, go-template, or go-template-file. To see the full output, use the -o json option.


What is the purpose of outputing a create command? Why would someone want to dump Processor <Name> created. to a file?

tcannon91 · 2024-12-20T20:28:37Z

internal/cli/streams/processor/create.go

+		},
+	}
+
+	cmd.Flags().StringVar(&opts.ProjectID, flag.ProjectID, "", usage.ProjectID)


Should we start using:

Suggested change

cmd.Flags().StringVar(&opts.ProjectID, flag.ProjectID, "", usage.ProjectID)

opts.AddProjectOptsFlags(cmd)

tcannon91 · 2024-12-20T20:33:20Z

internal/cli/streams/processor/create.go

+	_ = cmd.MarkFlagFilename(flag.File)
+
+	_ = cmd.MarkFlagRequired(flag.Instance)
+	_ = cmd.MarkFlagRequired(flag.File)


[nit] Is it a linting rule that makes us have to assign to something? I think it would be a little cleaner to do something like this:

Suggested change

_ = cmd.MarkFlagFilename(flag.File)

_ = cmd.MarkFlagRequired(flag.Instance)

_ = cmd.MarkFlagRequired(flag.File)

cmd.MarkFlagFilename(flag.File)

cmd.MarkFlagRequired(flag.Instance)

cmd.MarkFlagRequired(flag.File)

tcannon91 · 2024-12-20T20:44:21Z

docs/command/atlas-streams-processors-create.txt

+   * - -f, --file
+     - string
+     - true
+     - Path to a JSON configuration file that defines an Atlas Stream Processing connection.


Is this creating stream processor connection or a stream processor?

tcannon91 · 2024-12-20T20:47:39Z

internal/cli/streams/processor/create_test.go

+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "missing file")


Could these be combined?

Suggested change

require.Error(t, err)

assert.Contains(t, err.Error(), "missing file")

assert.Contains(t, err, "missing file")

Similar comment for all the error assertions

tcannon91 · 2024-12-20T20:51:38Z

internal/cli/streams/processor/create_test.go

+			},
+		}
+
+		name := "ExampleSP"


[nit] we should either inline this down where it is used or use string interpolation to inject this value into the string that is written to the config file so that it is forced to be aligned with the test input and the expected output. Assigning it to a variable that we don't reuse feels weird.

tcannon91 · 2024-12-20T20:57:07Z

internal/cli/streams/processor/create_test.go

+		if err := createOpts.Run(); err != nil {
+			t.Fatalf("Run() unexpected error: %v", err)
+		}
+		t.Log(buf.String())


Is this a debugging line? Maybe it should be removed?

tcannon91 · 2024-12-20T21:03:58Z

internal/cli/streams/processor/create_test.go

+			t.Fatalf("Run() unexpected error: %v", err)
+		}
+		t.Log(buf.String())
+		test.VerifyOutputTemplate(t, createTemplate, expected)


What does this assertion verify? It isn't clear what it actually does, we might want a comment here, especially because I think it might be asserting the same thing as the next line?

tcannon91

E2E tests reviewed

tcannon91 · 2024-12-20T21:19:35Z

test/e2e/atlas/data/create_streams_connection_atlas.json

[q] I am only seeing this used by changes from 8 months ago. Any ideas why it is being added in your PR/where it is coming from?

tcannon91 · 2024-12-20T21:23:49Z

test/e2e/atlas/streams_with_clusters_test.go

+		a.Equal(processorName, *processor.Name)
+	})
+
+	t.Run("Describe a stream processor with an atlas cluster sink", func(t *testing.T) {


All of the tests below this one concern me because they are relying on data populated/created by previous t.Run() tests. Each test should be able to function in isolation. If you want to rely on previous configuration, then it should be nested within the test.

I would recommend each of the commands describe, list, delete, start, stop do all of the creation/setup necessary and just don't run assertions on those steps (since the assertions are covered here).

An alternative idea is to create a larger E2E test that tests everything. It could create, do assertions on the creation; describe, do assertions on the describe; list and assert; start and assert; stop and assert; and delete and assert.

tcannon91

Delete and Describe reviewed

tcannon91 · 2024-12-20T21:28:00Z

internal/cli/streams/processor/delete.go

+	}
+	cmd := &cobra.Command{
+		Use:   "delete <processorName>",
+		Short: "Delete a specific Atlas Stream Processor in a Stream Processing Instance.",


[nit]

Suggested change

Short: "Delete a specific Atlas Stream Processor in a Stream Processing Instance.",

Short: "Delete an Atlas Stream Processor in a Stream Processing Instance.",

tcannon91 · 2024-12-20T21:29:17Z

internal/cli/streams/processor/delete.go

+	cmd := &cobra.Command{
+		Use:   "delete <processorName>",
+		Short: "Delete a specific Atlas Stream Processor in a Stream Processing Instance.",
+		Long:  fmt.Sprintf(usage.RequiredRole, "Project Read Only"),


This role doesn't seem correct.... I don't think you should be able to delete with a read only role.

tcannon91 · 2024-12-20T21:30:51Z

internal/cli/streams/processor/delete_test.go

+	"github.com/mongodb/mongodb-atlas-cli/atlascli/internal/mocks"
+)
+
+func TestDeleteOpts_Run(t *testing.T) {


There should be a t.Run() call in this test describing what is being tested/asserted.

tcannon91 · 2024-12-20T21:37:33Z

internal/cli/streams/processor/describe.go

+	opts := &DescribeOpts{}
+	cmd := &cobra.Command{
+		Use:   "describe <processorName>",
+		Short: "Get details about a specific Atlas Stream Processor in a Stream Processing Instance.",


[nit]

Suggested change

Short: "Get details about a specific Atlas Stream Processor in a Stream Processing Instance.",

Short: "Get details about an Atlas Stream Processor in a Stream Processing Instance.",

tcannon91 · 2024-12-20T21:39:00Z

internal/cli/streams/processor/describe.go

+func DescribeBuilder() *cobra.Command {
+	opts := &DescribeOpts{}
+	cmd := &cobra.Command{
+		Use:   "describe <processorName>",


fwiw, this command seems like it would make more sense to have a json output file that is the .json configuration file that would work with the create command (and eventually update).

tcannon91 · 2024-12-20T21:41:59Z

internal/cli/streams/processor/describe.go

+	if opts.includeStats {
+		return opts.Print(r)
+	}
+
+	sp := atlasv2.NewStreamsProcessorWithStats(r.Id, r.Name, r.Pipeline, r.State)
+	return opts.Print(sp)


I think the logic is backwards here... we should be returning early if !opts.includeStats

tcannon91 · 2024-12-20T21:44:08Z

internal/cli/streams/processor/describe.go

+		return opts.Print(r)
+	}
+
+	sp := atlasv2.NewStreamsProcessorWithStats(r.Id, r.Name, r.Pipeline, r.State)


I think this should be created through a store method. I am not familiar with the decision patterns, I have only seen the CLI interact with the api through stores rather than directly like this.

tcannon91 · 2024-12-20T21:46:05Z

internal/cli/streams/processor/describe_test.go

+	mockStore.
+		EXPECT().
+		StreamProcessor(gomock.Any()).
+		Return(expected, nil).
+		Times(2)


I think that separate stores should be pulled into each test to more specifically assert each test's code.

tcannon91

Start and Stop reviewed

tcannon91 · 2024-12-20T21:52:37Z

internal/cli/streams/processor/start.go

+	cmd := &cobra.Command{
+		Use:   "start <processorName>",
+		Short: "Start a specific Atlas Stream Processor in a Stream Processing Instance.",
+		Long:  fmt.Sprintf(usage.RequiredRole, "Project Read Only"),


The docs indicate this command will require either Project Owner or Project Stream Processing Owner

tcannon91 · 2024-12-20T21:54:55Z

internal/cli/streams/processor/start_test.go

+
+	mockStore.
+		EXPECT().
+		StartStreamProcessor(gomock.Any()).


I think we should have stricter assertions on what we pass to the store calls. Splitting the commands out into separate strings might make this easier to assert on then reconstructing Params objects. But it shouldn't be too hard either way.

auddin431 requested review from a team as code owners December 19, 2024 22:06

auddin431 requested review from tcannon91 and jarjee December 19, 2024 22:06

auddin431 added 2 commits December 19, 2024 17:23

CLOUDP-289219: Create CLI commands for manipulating stream processors

ba0eaaf

rebase to latest commit

e916573

auddin431 force-pushed the CLOUDP-289219 branch from 22ef83a to e916573 Compare December 19, 2024 22:42

tcannon91 requested changes Dec 20, 2024

View reviewed changes

tcannon91 reviewed Dec 20, 2024

View reviewed changes

tcannon91 requested changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLOUDP-289219: Create CLI commands for manipulating stream processors #3496

CLOUDP-289219: Create CLI commands for manipulating stream processors #3496

auddin431 commented Dec 19, 2024

tcannon91 left a comment •

edited

Loading

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 left a comment

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 left a comment

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

tcannon91 left a comment

tcannon91 Dec 20, 2024

tcannon91 Dec 20, 2024

	cmd.Flags().StringVar(&opts.ProjectID, flag.ProjectID, "", usage.ProjectID)
	opts.AddProjectOptsFlags(cmd)

		require.Error(t, err)
		assert.Contains(t, err.Error(), "missing file")

	require.Error(t, err)
	assert.Contains(t, err.Error(), "missing file")
	assert.Contains(t, err, "missing file")

	Short: "Delete a specific Atlas Stream Processor in a Stream Processing Instance.",
	Short: "Delete an Atlas Stream Processor in a Stream Processing Instance.",

	Short: "Get details about a specific Atlas Stream Processor in a Stream Processing Instance.",
	Short: "Get details about an Atlas Stream Processor in a Stream Processing Instance.",

CLOUDP-289219: Create CLI commands for manipulating stream processors #3496

Are you sure you want to change the base?

CLOUDP-289219: Create CLI commands for manipulating stream processors #3496

Conversation

auddin431 commented Dec 19, 2024

Proposed changes

Checklist

tcannon91 left a comment • 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

tcannon91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcannon91 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcannon91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcannon91 left a comment •

edited

Loading