-
-
Notifications
You must be signed in to change notification settings - Fork 580
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
fix: hide custom ddev pull and ddev push commands from Amplitude, fixes #6128 #6152
fix: hide custom ddev pull and ddev push commands from Amplitude, fixes #6128 #6152
Conversation
Download the artifacts for this pull request:
See Testing a PR. |
This looks fine to me, thanks. Does need manual testing. DDEV_VERBOSE=true might be good enough, but probably step-debugging it would be adequate as well. We can just rename one of the existing ones like lagoon or whatever and use it and see what happens. |
I tested what would be sent to Amplitude after my change:
"event_properties": {
"Arguments": null,
"Called As": "platform123",
"Command Name": "custom-command",
"Command Path": "ddev pull custom-command"
}
I think this is unintentional and should be fixed.
Lines 42 to 48 in 195334d
I tried to add ddev/pkg/amplitude/amplitude.go Lines 77 to 81 in 195334d
Like this: import (
cmd2 "github.com/ddev/ddev/cmd/ddev/cmd"
)
// Anonymize user defined custom commands.
calledAs := cmd.CalledAs()
if cmd2.IsUserDefinedCustomCommand(cmd) {
calledAs = "custom-command"
}
builder := ampli.Command.Builder().
Arguments(args).
CalledAs(calledAs).
CommandName(cmd.Name()).
CommandPath(cmd.CommandPath()) But without success because of What is the best way to add a check for custom commands in |
7c461ba
to
d9f66cc
Compare
I found the solution (assign a new command instead of reusing), now it sends it as: "event_properties": {
"Arguments": null,
"Called As": "",
"Command Name": "custom-command",
"Command Path": "ddev pull custom-command"
} |
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.
@@ -125,6 +125,13 @@ ddev pull %s --skip-files -y`, subCommandName, subCommandName, subCommandName), | |||
appPull(providerName, app, flags["skip-confirmation"], flags["skip-import"], flags["skip-db"], flags["skip-files"], environment) | |||
}, | |||
} | |||
// Mark custom command |
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.
Very small nit, not worth messing with.
// Mark custom command | |
// Mark custom provider |
func IsBundledCustomProvider(provider string) bool { | ||
paths := []string{ | ||
filepath.Join("dotddev_assets", "providers", provider) + ".yaml", | ||
filepath.Join("dotddev_assets", "providers", provider) + ".yaml.example", |
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.
This approach is fine. In reality, we probably wouldn't want to show any args to rsync
or localfile
, but let's just let that be.
The Issue
How This PR Solves The Issue
Marks the command as custom, but only if the name is not in predefined provider list (like acquia, git, lagoon, etc.)
Manual Testing Instructions
Tested:
Automated Testing Overview
Related Issue Link(s)
Release/Deployment Notes