From 49fccfc91ba1bae558b293adaa9c1e45338c6d47 Mon Sep 17 00:00:00 2001 From: Andrew Starr-Bochicchio Date: Fri, 8 Dec 2023 11:48:50 -0500 Subject: [PATCH] Resolve conflict with alert configurations by overriding command namespace (#1470) * Add integration tests for uptime alerts. * Remove unneeded logging from tests making output noisy. * Re-enable failing tests. * Add option to override the config namespace for a command. --- commands/command.go | 33 +- commands/command_option.go | 10 + commands/doit.go | 15 +- commands/doit_test.go | 91 +++++ commands/monitoring.go | 1 + commands/uptime_alerts.go | 12 +- do/uptime_checks.go | 1 + integration/database_firewall_update_test.go | 2 - integration/database_user_create_test.go | 1 - integration/monitoring_test.go | 12 +- integration/uptime_alert_test.go | 345 +++++++++++++++++++ 11 files changed, 492 insertions(+), 31 deletions(-) create mode 100644 commands/doit_test.go create mode 100644 integration/uptime_alert_test.go diff --git a/commands/command.go b/commands/command.go index 23d399858..88d1fa3c7 100644 --- a/commands/command.go +++ b/commands/command.go @@ -34,6 +34,10 @@ type Command struct { fmtCols []string childCommands []*Command + + // overrideNS specifies a namespace to use in config. + // Set using the overrideCmdNS cmdOption when calling CmdBuilder + overrideNS string } // AddCommand adds child commands and adds child commands for cobra as well. @@ -72,19 +76,6 @@ func cmdBuilderWithInit(parent *Command, cr CmdRunner, cliText, shortdesc string Use: cliText, Short: shortdesc, Long: longdesc, - Run: func(cmd *cobra.Command, args []string) { - c, err := NewCmdConfig( - cmdNS(cmd), - &doctl.LiveConfig{}, - out, - args, - initCmd, - ) - checkErr(err) - - err = cr(c) - checkErr(err) - }, } c := &Command{Command: cc} @@ -97,6 +88,22 @@ func cmdBuilderWithInit(parent *Command, cr CmdRunner, cliText, shortdesc string co(c) } + // This must be defined after the options have been applied + // so that changes made by the options are accessible here. + c.Command.Run = func(cmd *cobra.Command, args []string) { + c, err := NewCmdConfig( + cmdNS(c), + &doctl.LiveConfig{}, + out, + args, + initCmd, + ) + checkErr(err) + + err = cr(c) + checkErr(err) + } + if cols := c.fmtCols; cols != nil { formatHelp := fmt.Sprintf("Columns for output in a comma-separated list. Possible values: `%s`.", strings.Join(cols, "`"+", "+"`")) diff --git a/commands/command_option.go b/commands/command_option.go index fca46dd8d..66ba75698 100644 --- a/commands/command_option.go +++ b/commands/command_option.go @@ -42,3 +42,13 @@ func hiddenCmd() cmdOption { c.Hidden = true } } + +// overrideCmdNS specifies a namespace to use in config overriding the +// normal usage of the parent command's name. This is useful in cases +// where deeply nested subcommands have conflicting names. See uptime_alerts.go +// for example usage. +func overrideCmdNS(ns string) cmdOption { + return func(c *Command) { + c.overrideNS = ns + } +} diff --git a/commands/doit.go b/commands/doit.go index fdda3360a..4a2d3ef01 100644 --- a/commands/doit.go +++ b/commands/doit.go @@ -326,16 +326,25 @@ func AddDurationFlag(cmd *Command, name, shorthand string, def time.Duration, de func flagName(cmd *Command, name string) string { if cmd.Parent() != nil { - return fmt.Sprintf("%s.%s.%s", cmd.Parent().Name(), cmd.Name(), name) + p := cmd.Parent().Name() + if cmd.overrideNS != "" { + p = cmd.overrideNS + } + return fmt.Sprintf("%s.%s.%s", p, cmd.Name(), name) } + return fmt.Sprintf("%s.%s", cmd.Name(), name) } -func cmdNS(cmd *cobra.Command) string { +func cmdNS(cmd *Command) string { if cmd.Parent() != nil { + if cmd.overrideNS != "" { + return fmt.Sprintf("%s.%s", cmd.overrideNS, cmd.Name()) + } + return fmt.Sprintf("%s.%s", cmd.Parent().Name(), cmd.Name()) } - return fmt.Sprintf("%s", cmd.Name()) + return cmd.Name() } func isTerminal(f *os.File) bool { diff --git a/commands/doit_test.go b/commands/doit_test.go new file mode 100644 index 000000000..e2049ba62 --- /dev/null +++ b/commands/doit_test.go @@ -0,0 +1,91 @@ +package commands + +import ( + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" +) + +func TestFlagName(t *testing.T) { + var flag = "thing" + testFn := func(c *CmdConfig) error { + return nil + } + parent := &Command{ + Command: &cobra.Command{ + Use: "doit", + Short: "Do the thing", + }, + } + + tests := []struct { + name string + cmd *Command + expected string + }{ + { + name: "default", + cmd: CmdBuilder(parent, testFn, "run", "Run it", "", Writer), + expected: "doit.run.thing", + }, + { + name: "top-level", + cmd: parent, + expected: "doit.thing", + }, + { + name: "overrideCmdNS", + cmd: CmdBuilder(parent, testFn, "run", "Run it", "", Writer, overrideCmdNS("doctl")), + expected: "doctl.run.thing", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + AddStringFlag(tt.cmd, flag, "", "", "the thing") + + assert.Equal(t, tt.expected, flagName(tt.cmd, flag)) + }) + } +} + +func TestCmdNS(t *testing.T) { + testFn := func(c *CmdConfig) error { + return nil + } + parent := &Command{ + Command: &cobra.Command{ + Use: "doit", + Short: "Do the thing", + }, + } + + tests := []struct { + name string + cmd *Command + expected string + }{ + { + name: "default", + cmd: CmdBuilder(parent, testFn, "run", "Run it", "", Writer), + expected: "doit.run", + }, + { + name: "top-level", + cmd: parent, + expected: "doit", + }, + { + name: "overrideCmdNS", + cmd: CmdBuilder(parent, testFn, "run", "Run it", "", Writer, overrideCmdNS("doctl")), + expected: "doctl.run", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, cmdNS(tt.cmd)) + }) + } +} diff --git a/commands/monitoring.go b/commands/monitoring.go index e520fa23d..56a2195d0 100644 --- a/commands/monitoring.go +++ b/commands/monitoring.go @@ -110,6 +110,7 @@ func RunCmdAlertPolicyCreate(c *CmdConfig) error { if err != nil { return err } + err = validateAlertPolicyType(alertType) if err != nil { return err diff --git a/commands/uptime_alerts.go b/commands/uptime_alerts.go index d3a49696b..0d156c171 100644 --- a/commands/uptime_alerts.go +++ b/commands/uptime_alerts.go @@ -29,9 +29,10 @@ import ( func UptimeAlert() *Command { cmd := &Command{ Command: &cobra.Command{ - Use: "alert", - Short: "Display commands to manage uptime alerts", - Long: `The sub-commands of ` + "`" + `doctl uptime alert` + "`" + ` manage your uptime alerts. + Use: "alert", + Aliases: []string{"alerts"}, + Short: "Display commands to manage uptime alerts", + Long: `The sub-commands of ` + "`" + `doctl monitoring uptime alert` + "`" + ` manage your uptime alerts. DigitalOcean Uptime Alerts provide the ability to monitor your endpoints from around the world, and alert you when they're slow, unavailable, or SSL certificates are expiring.`, @@ -41,7 +42,8 @@ and alert you when they're slow, unavailable, or SSL certificates are expiring.` cmdUptimeAlertsCreate := CmdBuilder(cmd, RunUptimeAlertsCreate, "create ", "Create an uptime alert", `Use this command to create an uptime alert on your account. You can use flags to specify the uptime alert, type, threshold, comparison, notifications, and period.`, Writer, - aliasOpt("c"), displayerType(&displayers.UptimeAlert{})) + aliasOpt("c"), displayerType(&displayers.UptimeAlert{}), overrideCmdNS("uptime-alert")) + AddStringFlag(cmdUptimeAlertsCreate, doctl.ArgUptimeAlertName, "", "", "Uptime alert name", requiredOpt()) AddStringFlag(cmdUptimeAlertsCreate, doctl.ArgUptimeAlertType, "", "", "Uptime alert type, must be one of latency, down, down_global or ssl_expiry", requiredOpt()) AddIntFlag(cmdUptimeAlertsCreate, doctl.ArgUptimeAlertThreshold, "", 0, "Uptime alert threshold at which the alert will enter a trigger state. The specific threshold is dependent on the alert type.") @@ -60,7 +62,7 @@ You can use flags to specify the uptime alert, type, threshold, comparison, noti cmdUptimeAlertsUpdate := CmdBuilder(cmd, RunUptimeAlertsUpdate, "update ", "Update an uptime alert", `Use this command to update an uptime alert on your account. You can use flags to specify the uptime alert, type, threshold, comparison, notifications, and period.`, Writer, - aliasOpt("u"), displayerType(&displayers.UptimeAlert{})) + aliasOpt("u"), displayerType(&displayers.UptimeAlert{}), overrideCmdNS("uptime-alert")) AddStringFlag(cmdUptimeAlertsUpdate, doctl.ArgUptimeAlertName, "", "", "Uptime alert name", requiredOpt()) AddStringFlag(cmdUptimeAlertsUpdate, doctl.ArgUptimeAlertType, "", "", "Uptime alert type, must be one of latency, down, down_global or ssl_expiry", requiredOpt()) AddIntFlag(cmdUptimeAlertsUpdate, doctl.ArgUptimeAlertThreshold, "", 0, "Uptime alert threshold at which the alert will enter a trigger state. The specific threshold is dependent on the alert type.") diff --git a/do/uptime_checks.go b/do/uptime_checks.go index 3161cbba9..5804d91ce 100644 --- a/do/uptime_checks.go +++ b/do/uptime_checks.go @@ -132,6 +132,7 @@ func (ucs *uptimeChecksService) CreateAlert(id string, req *godo.CreateUptimeAle if err != nil { return nil, err } + return &UptimeAlert{uptimeAlert}, nil } diff --git a/integration/database_firewall_update_test.go b/integration/database_firewall_update_test.go index 9f2a0b442..dde2ad10a 100644 --- a/integration/database_firewall_update_test.go +++ b/integration/database_firewall_update_test.go @@ -32,8 +32,6 @@ var _ = suite("database/firewalls", func(t *testing.T, when spec.G, it spec.S) { if req.Method == http.MethodPut { reqBody, err := io.ReadAll(req.Body) expect.NoError(err) - t.Log(string(reqBody)) - t.Log(databasesUpdateFirewallUpdateRequest) expect.JSONEq(databasesUpdateFirewallUpdateRequest, string(reqBody)) w.Write([]byte(databasesUpdateFirewallRuleResponse)) } else if req.Method == http.MethodGet { diff --git a/integration/database_user_create_test.go b/integration/database_user_create_test.go index f158c8b9a..f983f8bb4 100644 --- a/integration/database_user_create_test.go +++ b/integration/database_user_create_test.go @@ -101,7 +101,6 @@ var _ = suite("database/user/create", func(t *testing.T, when spec.G, it spec.S) ) output, err := cmd.CombinedOutput() - t.Log(string(output)) expect.NoError(err, fmt.Sprintf("received error output: %s", output)) expect.Equal(strings.TrimSpace(databaseUserCreateOutput), strings.TrimSpace(string(output))) }) diff --git a/integration/monitoring_test.go b/integration/monitoring_test.go index 790d77633..4d494f836 100644 --- a/integration/monitoring_test.go +++ b/integration/monitoring_test.go @@ -60,7 +60,7 @@ var _ = suite("monitoring/alerts/get", func(t *testing.T, when spec.G, it spec.S ) output, err := cmd.CombinedOutput() - expect.NoError(err) + expect.NoError(err, string(output)) expect.Equal(strings.TrimSpace(getAlertPolicyOutput), strings.TrimSpace(string(output))) }) @@ -94,8 +94,7 @@ UUID Type Description ` ) -// TODO: Re-enable test -var _ = suite.Pend("monitoring/alerts/create", func(t *testing.T, when spec.G, it spec.S) { +var _ = suite("monitoring/alerts/create", func(t *testing.T, when spec.G, it spec.S) { var ( expect *require.Assertions server *httptest.Server @@ -155,7 +154,7 @@ var _ = suite.Pend("monitoring/alerts/create", func(t *testing.T, when spec.G, i ) output, err := cmd.CombinedOutput() - expect.NoError(err) + expect.NoError(err, string(output)) expect.Equal(strings.TrimSpace(createAlertPolicyOutput), strings.TrimSpace(string(output))) }) @@ -189,8 +188,7 @@ UUID Type Description ` ) -// TODO: Re-enable test -var _ = suite.Pend("monitoring/alerts/update", func(t *testing.T, when spec.G, it spec.S) { +var _ = suite("monitoring/alerts/update", func(t *testing.T, when spec.G, it spec.S) { var ( expect *require.Assertions server *httptest.Server @@ -250,7 +248,7 @@ var _ = suite.Pend("monitoring/alerts/update", func(t *testing.T, when spec.G, i ) output, err := cmd.CombinedOutput() - expect.NoError(err) + expect.NoError(err, string(output)) expect.Equal(strings.TrimSpace(updateAlertPolicyOutput), strings.TrimSpace(string(output))) }) diff --git a/integration/uptime_alert_test.go b/integration/uptime_alert_test.go new file mode 100644 index 000000000..daab3af8a --- /dev/null +++ b/integration/uptime_alert_test.go @@ -0,0 +1,345 @@ +package integration + +import ( + "net/http" + "net/http/httptest" + "net/http/httputil" + "os/exec" + "strings" + "testing" + + "github.com/sclevine/spec" + "github.com/stretchr/testify/require" +) + +var _ = suite("monitoring/uptime/alerts/get", func(t *testing.T, when spec.G, it spec.S) { + var ( + expect *require.Assertions + server *httptest.Server + ) + + it.Before(func() { + expect = require.New(t) + + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Add("content-type", "application/json") + + switch req.URL.Path { + case "/v2/uptime/checks/valid-check-uuid/alerts/valid-alert-uuid": + auth := req.Header.Get("Authorization") + if auth != "Bearer some-magic-token" { + w.WriteHeader(http.StatusUnauthorized) + return + } + + if req.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + + w.Write([]byte(uptimeAlertGetResponse)) + default: + dump, err := httputil.DumpRequest(req, true) + if err != nil { + t.Fatal("failed to dump request") + } + + t.Fatalf("received unknown request: %s", dump) + } + })) + }) + + it("returns the details of the uptime alert", func() { + cmd := exec.Command(builtBinaryPath, + "-t", "some-magic-token", + "-u", server.URL, + "monitoring", + "uptime", + "alert", + "get", + "valid-check-uuid", + "valid-alert-uuid", + ) + + output, err := cmd.CombinedOutput() + expect.NoError(err) + + expect.Equal(strings.TrimSpace(uptimeAlertGetOutput), strings.TrimSpace(string(output))) + }) +}) + +const ( + uptimeAlertGetResponse = ` +{ + "alert": { + "id": "valid-alert-uuid", + "name": "example.com is down", + "type": "down", + "threshold": 1, + "comparison": "less_than", + "notifications": { + "email": [ + "sammy@digitalocean.com" + ], + "slack": [ + { + "channel": "#alerts", + "url": "https://hooks.slack.com/services/ID" + } + ] + }, + "period": "2m" + } +}` + uptimeAlertGetOutput = ` +ID Name Type Threshold Comparison Period Emails Slack Channels +valid-alert-uuid example.com is down down 1 less_than 2m sammy@digitalocean.com #alerts +` +) + +var _ = suite("monitoring/uptime/alerts/list", func(t *testing.T, when spec.G, it spec.S) { + var ( + expect *require.Assertions + server *httptest.Server + ) + + it.Before(func() { + expect = require.New(t) + + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Add("content-type", "application/json") + + switch req.URL.Path { + case "/v2/uptime/checks/valid-check-uuid/alerts": + auth := req.Header.Get("Authorization") + if auth != "Bearer some-magic-token" { + w.WriteHeader(http.StatusUnauthorized) + return + } + + if req.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + + w.Write([]byte(uptimeAlertListResponse)) + default: + dump, err := httputil.DumpRequest(req, true) + if err != nil { + t.Fatal("failed to dump request") + } + + t.Fatalf("received unknown request: %s", dump) + } + })) + }) + + it("lists all uptime alerts", func() { + cmd := exec.Command(builtBinaryPath, + "-t", "some-magic-token", + "-u", server.URL, + "monitoring", + "uptime", + "alert", + "list", + "valid-check-uuid", + ) + + output, err := cmd.CombinedOutput() + expect.NoError(err, string(output)) + + expect.Equal(strings.TrimSpace(uptimeAlertListOutput), strings.TrimSpace(string(output))) + }) +}) + +const ( + uptimeAlertListResponse = ` +{ + "alerts":[ + { + "id": "valid-alert-uuid", + "name": "example.com is down", + "type": "down", + "threshold": 1, + "comparison": "less_than", + "notifications": { + "email": [ + "sammy@digitalocean.com" + ], + "slack": [ + { + "channel": "#alerts", + "url": "https://hooks.slack.com/services/ID" + } + ] + }, + "period": "2m" + }, + { + "id": "fee528f9-73a2-46a9-a248-84056c9a4488", + "name": "example.com increased latency", + "type": "latency", + "threshold": 1000, + "comparison": "greater_than", + "notifications": { + "email": [ + "sammy@digitalocean.com" + ] + }, + "period": "2m" + } + ] +}` + uptimeAlertListOutput = ` +ID Name Type Threshold Comparison Period Emails Slack Channels +valid-alert-uuid example.com is down down 1 less_than 2m sammy@digitalocean.com #alerts +fee528f9-73a2-46a9-a248-84056c9a4488 example.com increased latency latency 1000 greater_than 2m sammy@digitalocean.com +` +) + +var _ = suite("monitoring/uptime/alerts/create", func(t *testing.T, when spec.G, it spec.S) { + var ( + expect *require.Assertions + server *httptest.Server + ) + + it.Before(func() { + expect = require.New(t) + + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Add("content-type", "application/json") + + switch req.URL.Path { + case "/v2/uptime/checks/valid-check-uuid/alerts": + auth := req.Header.Get("Authorization") + if auth != "Bearer some-magic-token" { + w.WriteHeader(http.StatusUnauthorized) + return + } + + if req.Method != http.MethodPost { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + + w.Write([]byte(uptimeAlertPolicyCreateResponse)) + default: + dump, err := httputil.DumpRequest(req, true) + if err != nil { + t.Fatal("failed to dump request") + } + + t.Fatalf("received unknown request: %s", dump) + } + })) + }) + + it("creates a new uptime alert", func() { + cmd := exec.Command(builtBinaryPath, + "-t", "some-magic-token", + "-u", server.URL, + "monitoring", + "uptime", + "alerts", + "create", + "--comparison", "less_than", + "--type", "down", + "--period", "2m", + "--name", "example.com is down", + "--emails", "sammy@digitalocean.com", + "--threshold", "1", + "--slack-channels", "#alerts", + "--slack-urls", "https://hooks.slack.com/services/ID", + "valid-check-uuid", + ) + + output, err := cmd.CombinedOutput() + expect.NoError(err, string(output)) + + expect.Equal(strings.TrimSpace(createUptimeAlertPolicyOutput), strings.TrimSpace(string(output))) + }) +}) + +const ( + uptimeAlertPolicyCreateResponse = `{ + "alert": { + "id": "valid-alert-uuid", + "name": "example.com is down", + "type": "down", + "threshold": 1, + "comparison": "less_than", + "notifications": { + "email": [ + "sammy@digitalocean.com" + ], + "slack": [ + { + "channel": "#alerts", + "url": "https://hooks.slack.com/services/ID" + } + ] + }, + "period": "2m" + } +}` + + createUptimeAlertPolicyOutput = ` +ID Name Type Threshold Comparison Period Emails Slack Channels +valid-alert-uuid example.com is down down 1 less_than 2m sammy@digitalocean.com #alerts` +) + +var _ = suite("monitoring/uptime/alerts/delete", func(t *testing.T, when spec.G, it spec.S) { + var ( + expect *require.Assertions + cmd *exec.Cmd + server *httptest.Server + ) + + it.Before(func() { + expect = require.New(t) + + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + switch req.URL.Path { + case "/v2/uptime/checks/valid-check-uuid/alerts/valid-alert-uuid": + auth := req.Header.Get("Authorization") + if auth != "Bearer some-magic-token" { + w.WriteHeader(http.StatusUnauthorized) + return + } + + if req.Method != http.MethodDelete { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + + w.WriteHeader(http.StatusNoContent) + default: + dump, err := httputil.DumpRequest(req, true) + if err != nil { + t.Fatal("failed to dump request") + } + + t.Fatalf("received unknown request: %s", dump) + } + })) + + }) + + when("required flags are passed", func() { + it("deletes the uptime alert", func() { + cmd = exec.Command(builtBinaryPath, + "-t", "some-magic-token", + "-u", server.URL, + "monitoring", + "uptime", + "alert", + "delete", + "valid-check-uuid", + "valid-alert-uuid", + ) + output, err := cmd.CombinedOutput() + expect.NoError(err) + expect.Empty(output) + }) + }) +})