CLOUDP-237254: Update remaining commands to append deployment type telemetry #2803
Conversation
blva
changed the title
blva
changed the title
internal/cli/deployments/connect.go
Outdated
| @@ -28,6 +28,11 @@ func Run(ctx context.Context, opts *options.ConnectOpts) error { | |||
| return opts.Connect(ctx) | |||
| } | |||
|
|
|||
| func PostRun(opts *options.ConnectOpts) error { | |||
There was a problem hiding this comment.
why not
| func PostRun(opts *options.ConnectOpts) error { | |
| func (opts *options.ConnectOpts) PostRun() error { |
There was a problem hiding this comment.
we can't define it there in connect.go and I found too weird to define it in the opts file, so I followed same run convention, I tried this approach before but didn't like it so kept this way
There was a problem hiding this comment.
it seems to me from a distance we may be falling into the same traps that made the setup command unmaintainable with complicated "inheritance" that's making commands hard to modify
There was a problem hiding this comment.
I think what we can do here, is to move everything from connect_opts_atlas.go and connect_opts_local.go into connect.go then, would that make sense? if so, I can do that in a next PR to avoid growing this one, I can start the draft now
There was a problem hiding this comment.
Let's open a ticket and discuss as a team if worth the effort given we plan to re write a lot of this logic
There was a problem hiding this comment.
| @@ -53,6 +57,9 @@ func ConnectBuilder() *cobra.Command { | |||
| RunE: func(cmd *cobra.Command, _ []string) error { | |||
| return Run(cmd.Context(), opts) | |||
| }, | |||
| PostRun: func(_ *cobra.Command, _ []string) { | |||
| PostRun(opts) | |||
There was a problem hiding this comment.
| PostRun(opts) | |
| opts.DeploymentTelemetry.AppendDeploymentType() |
There was a problem hiding this comment.
we need it there so that we can test it
| AppendDeploymentType(). | ||
| Times(1) | ||
|
|
||
| PostRun(opts) |
There was a problem hiding this comment.
given this test doesn't test much left a comment to get rid of PostRun
There was a problem hiding this comment.
this is a way to future proof changes and making sure we always capture telemetry, with time maybe more values will be added here, but id'like to keep them tested
There was a problem hiding this comment.
merging but feel free to pushback if there are any other reasons
Proposed changes
Jira ticket: CLOUDP-237254
Checklist
make fmtand formatted my codeFurther comments