-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Create advancedClusterToV2 command skeleton #58
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR creates the skeleton for a new advancedClusterToNew
command that will convert MongoDB Atlas advanced cluster configurations from provider version 1.X.X (SDKv2) to version 2.X.X (TPF - Terraform Plugin Framework). The implementation focuses on establishing the command structure and test infrastructure while deferring the actual transformation logic to future PRs.
- Adds the new
adv2new
command with basic CLI structure and parameter validation - Refactors existing code to extract common functionality shared between CLI commands
- Creates comprehensive test infrastructure for both unit and e2e testing of the new command
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
cmd/plugin/main.go |
Registers the new adv2new command with the CLI |
internal/cli/common.go |
Extracts shared CLI functionality into reusable BaseOpts struct |
internal/cli/clu2adv/clu2adv.go |
Refactors to use shared CLI functionality |
internal/cli/adv2new/adv2new.go |
Implements the new command using shared CLI patterns |
internal/convert/adv2new.go |
Creates placeholder conversion function |
test/e2e/e2e_helper.go |
Adds shared test infrastructure for both commands |
test/e2e/clu2adv_test.go |
Refactors to use shared test infrastructure |
test/e2e/adv2new_test.go |
Implements e2e tests for the new command |
internal/convert/convert_test.go |
Refactors to support testing multiple commands |
Various testdata files | Adds test fixtures for the new command |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
internal/cli/adv2new/adv2new.go
Outdated
Convert: convert.AdvancedClusterToNew, | ||
} | ||
cmd := &cobra.Command{ | ||
Use: "advancedClusterToNew", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use: "advancedClusterToNew", | |
Use: "advancedClusterV2", |
just for sanity, how would the full command look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some equivalent examples to use it with names or aliases are:
atlas tf adv2new -f example.in.tf -o example.out.tf
atlas terraform advancedClusterToNew --file example.in.tf --output example.out.tf
# current cluster to adv_cluster command
atlas tf clu2adv -f example.in.tf -o example.out.tf
atlas terraform clusterToAdvancedCluster --file example.in.tf --output example.out.tf
you can also mix names and aliases in the same command
I like the convert command pattern name of xxxToyyy
and alias x2y
, maybe advancedClusterToV2
and adv2v2
? (not convinced with so many 2, that's why i tried to avoid 2 in the command name).
Having this context, what command and alias do you think it's better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcosuma let me know if it's ok that I keep the current name and alias at the moment and I merge the PR as it is, or you prefer to use a different alias and name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking this PR, but I think New
and Latest
quickly become obsolete or ambiguous, that's why ...ToV2
I like it more. 2v2
is not ideal, but that's also an alias so I am less concerned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fair, changed here: 32398ac
@@ -0,0 +1,130 @@ | |||
package cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice re-use!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent re-use!
@EspenAlbert thanks :-) it's what took most of the time in the PR |
Description
Create advancedClusterToV2 command skeleton.
Link to any related issue(s): CLOUDP-337602
Type of change:
Required Checklist:
Further comments