-
Notifications
You must be signed in to change notification settings - Fork 1
chore: Create plugin command with test infra #12
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
This PR has gone 7 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 7 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! |
return o.Run() | ||
}, | ||
} | ||
cmd.Flags().StringVarP(&o.file, "file", "f", "", "input file") |
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.
I think that at the moment it's not necessary to have const files for Flag, Usage, etc.
@@ -4,7 +4,7 @@ import ( | |||
"fmt" | |||
"os" | |||
|
|||
"github.com/mongodb-labs/atlas-cli-plugin-terraform/internal/cli/hello" | |||
"github.com/mongodb-labs/atlas-cli-plugin-terraform/internal/cli/clu2adv" |
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.
A bit unconventional package name, but I like it
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.
it's the (short) alias for the command, but let me know if you think of a better name
require ( | ||
github.com/hashicorp/hcl/v2 v2.23.0 | ||
github.com/sebdah/goldie/v2 v2.5.5 | ||
github.com/spf13/afero v1.12.0 |
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.
Looks like a solid choice for fs handling
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.
yes, same as in CLI
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestClusterToAdvancedCluster(t *testing.T) { |
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! This will be easy to extend
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.
yes, just drop the .in.tf , use make test-update, and check the output files are as expected, and there you have more tests
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.
Great job!
internal/hcl/hcl.go
Outdated
isResource := resource.Type() == "resource" | ||
labels := resource.Labels() | ||
resourceName := labels[0] | ||
if !isResource || resourceName != "mongodbatlas_cluster" { |
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.
can you make const strings from this file? mongodbatlas_cluster
, mongodbatlas_advanced_cluster
, resource
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.
thanks, changed here: db532d0
internal/cli/clu2adv/clu2adv.go
Outdated
o := &opts{fs: afero.NewOsFs()} | ||
cmd := &cobra.Command{ | ||
Use: "cluster_to_advanced", | ||
Short: "Upgrade cluster to advanced_cluster", |
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.
Upgrades?
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.
isn't actually Converts
more accurate?
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.
you're right, changed here: 1c24a51
internal/cli/clu2adv/clu2adv.go
Outdated
cmd := &cobra.Command{ | ||
Use: "cluster_to_advanced", | ||
Short: "Upgrade cluster to advanced_cluster", | ||
Long: "Upgrade Terraform config from mongodbatlas_cluster to mongodbatlas_advanced_cluster", |
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.
Long: "Upgrade Terraform config from mongodbatlas_cluster to mongodbatlas_advanced_cluster", | |
Long: "Converts a Terraform configuration from mongodbatlas_cluster to mongodbatlas_advanced_cluster", |
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.
changed here: 1c24a51
(keeping 2nd person form to be consistent)
_ = cmd.MarkFlagRequired("file") | ||
cmd.Flags().StringVarP(&o.output, "output", "o", "", "output file") | ||
_ = cmd.MarkFlagRequired("output") | ||
cmd.Flags().BoolVarP(&o.overwriteOutput, "overwriteOutput", "w", false, "overwrite output file if exists") |
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.
file
and output
are the standard params for input and output files
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.
lgtm
Co-authored-by: carriecwk <[email protected]>
Description
Create plugin command with test infra
Link to any related issue(s): CLOUDP-295406
Type of change:
Required Checklist:
Further comments