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

internal/store/streams.go

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?

You can set the output to JSON, i.e. -o json. I don't think anyone would dump the Processor <Name> created. to a file, but IMO seeing the JSON output could be valuable as a kind of verification that the correct processor was created (i.e proper name and pipeline)

Ah, I think I was confused because the output section here in the docs does not indicate that it prints the processor configuration. It just says Processor <Name> created.

internal/cli/streams/processor/create.go

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

internal/cli/streams/processor/create_test.go

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?

good catch! the e2e test generates this file dynamically here:

mongodb-atlas-cli/test/e2e/atlas/streams_with_clusters_test.go

Lines 128 to 133 in 77b0248

const configFile = "data/create_streams_connection_atlas.json"

file, err := os.Create(configFile)

if err != nil {

return "", err

}

defer file.Close()

When I ran the test locally, I forgot to clean it up so it was added to the PR as a new file

Nice! Should we add it to the .gitignore file for the repo so it doesn't accidentally get added back in the future?

Hmm, maybe instead of adding to gitignore we can do a better cleanup within the tests. My guess is that since these E2E tests run in Evergreen, the environment gets wiped at the end of the test so there wasn't much consideration for locally running the tests

test/e2e/atlas/streams_with_clusters_test.go

tcannon91

Delete and Describe reviewed

internal/cli/streams/processor/delete.go

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.

you're right, I will update the commands with the required roles provided in the API docs

I haven't opened the editor to look for other examples but is there a way to specify two potential roles? I see that everything has been updated to Project Owner but all the commands seem to support Project Owner OR Project Stream Processing Owner roles

So here's what the RequiredRole is defined as:

mongodb-atlas-cli/internal/usage/usage.go

Line 337 in 77b0248

RequiredRole = "To use this command, you must authenticate with a user account or an API key with the %s role."

I suppose I could add a new variable RequiredRoles that would go something like:

RequiredRoles = "To use this command, you must authenticate with a user account or an API key with the %s or %s role."

I'm a little uncertain about this because what if we add a command that can use one of three roles? or four? Alternatively, we can write a Streams specific variable for usage and it can live in the internal/cli/streams package or something

We could do something like create a new string format like this:

RequiredOneOfRoles = "To use this command, you must authenticate with a user account or an API key with any of the following roles: %s.";

then use a String.join(", ", ...roleList); and pass that as the string variable.

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.

[nit] t.Run is for test cases, if you don't have test cases please don't add it the name of the Test is what's under test, TestDeleteOpts_Run tells me is test the method Run from the DeleteOpts struct, editors like intellij will even enable auto navigation between methods and test by following the naming convention for tests

I think I agree with most of what you are saying. The information that I am missing that a t.Run() (or multiple) could provide is the expected behavior here. When trying to debug why this is failing in the future, I have no context as to what the expected behavior is. I could go through the assertions but it is then not clear if the assertion is no longer valid or the setup.

internal/cli/streams/processor/describe.go

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

hmm, i think the output of this command would work with create depending on the marshaling (if it ignores all the extra fields it should be fine). only issue is that the API call for creating a stream processor requires a StreamsProcessor struct, not the StreamsProcessorWithStats struct, but we need the stats for the describe command

internal/cli/streams/processor/describe.go

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.

that's a good point, but all I'm trying to do here is create the struct. This doesn't make any API calls, so I'm unsure if it should be a store method. @gssbzn is there a best practice that I should follow here?

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.

hmm, do you mean importing the actual store.ProcessorDescriber? im not sure how that would work because we need to mock the output of the API calls

No, I am suggestion that we should set up the mockStore in each t.Run() rather than defining it once outside the t.Run. This is an assertion that is not clearly captured in the individual test code and now we are asserting that it is called twice because we have 2 tests.

tcannon91

Start and Stop reviewed

internal/cli/streams/processor/start.go

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.

im not sure if that's entirely necessary because a lot of times, especially in the case of this test, we won't use those parameters. i might be misunderstanding what you're suggesting though

I am suggesting that we should assert that the projectId, tenantName, and processorName that are passed into this store function match the values passed into the cli command (or the expected values). As it stands, this does not verify the inputs do what is expected so if someone were to accidentally hardcode a tenantName in the cli command to accelerate their testing and it was missed during code review then we would ship this without knowing because the test does not assert the inputs to the cli command are passed all the way through to the store.

github-actions · 2025-01-20T00:04:30Z

This PR has gone 30 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 30 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy!

gssbzn · 2025-01-21T09:30:59Z

@auddin431 this is currently mark as stale by our bots, are you still working on this?

auddin431 · 2025-01-22T21:48:58Z

@auddin431 this is currently mark as stale by our bots, are you still working on this?

@gssbzn yep im still working on this, was out for a while during holidays and got occupied with other tickets. Will push out requested changes in the next day or two

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

github-actions bot added the stale A stale issue or PR label Jan 20, 2025

github-actions bot removed the stale A stale issue or PR label Jan 22, 2025

auddin431 added 3 commits January 22, 2025 15:49

CLOUDP-289219: Create CLI commands for manipulating stream processors

c9c9cbe

rebase to latest commit

c437c96

fix function arguments

ebac9cc

pr comments

c9887a4

auddin431 force-pushed the CLOUDP-289219 branch from e916573 to c9887a4 Compare January 22, 2025 23:28

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 •

edited

Loading

tcannon91 left a comment •

edited

Loading

tcannon91 Dec 20, 2024

auddin431 Jan 22, 2025

tcannon91 Jan 22, 2025

tcannon91 left a comment

tcannon91 Dec 20, 2024

auddin431 Jan 22, 2025

tcannon91 Jan 23, 2025

auddin431 Jan 23, 2025

tcannon91 left a comment

tcannon91 Dec 20, 2024

auddin431 Jan 22, 2025

tcannon91 Jan 23, 2025

auddin431 Jan 23, 2025

tcannon91 Jan 23, 2025

tcannon91 Dec 20, 2024

gssbzn Jan 21, 2025

tcannon91 Jan 21, 2025

tcannon91 Dec 20, 2024

auddin431 Jan 22, 2025

tcannon91 Dec 20, 2024

auddin431 Jan 22, 2025

tcannon91 Dec 20, 2024

auddin431 Jan 22, 2025

tcannon91 Jan 23, 2025

tcannon91 left a comment

tcannon91 Dec 20, 2024

auddin431 Jan 22, 2025

tcannon91 Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025

gssbzn commented Jan 21, 2025

auddin431 commented Jan 22, 2025

	const configFile = "data/create_streams_connection_atlas.json"
	file, err := os.Create(configFile)
	if err != nil {
	return "", err
	}
	defer file.Close()

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 • edited Loading

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

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

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

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 Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2025

gssbzn commented Jan 21, 2025

auddin431 commented Jan 22, 2025

auddin431 commented Dec 19, 2024 •

edited

Loading

tcannon91 left a comment •

edited

Loading

tcannon91 Jan 23, 2025 •

edited

Loading