-
Notifications
You must be signed in to change notification settings - Fork 55
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
DXCDT-386: Expand logs test cases #663
Conversation
69f58ab
to
1455482
Compare
1455482
to
a5d1b55
Compare
return nil, fmt.Errorf( | ||
"there are currently no log streams of type: %q, use 'auth0 logs streams create %s' to create one", | ||
desiredType, | ||
desiredType, | ||
) |
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.
We always give command hints in other places, so this just makes things consistent.
internal/cli/log_streams_datadog.go
Outdated
if oldLogStream.GetType() != string(logStreamTypeDatadog) { | ||
return fmt.Errorf( | ||
"the log stream with ID %q is of type %q instead of datadog, "+ | ||
"use 'auth0 logs streams update %s' to update it instead", | ||
inputs.ID, | ||
oldLogStream.GetType(), | ||
oldLogStream.GetType(), | ||
) | ||
} | ||
|
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.
Here we prevent a panic down below when we type cast the sink property, this safeguards us against passing a real log stream ID but of different type. This is replicated in all the update commands.
@@ -97,7 +97,7 @@ func updateLogStreamsAmazonEventBridgeCmd(cli *cli) *cobra.Command { | |||
|
|||
cmd := &cobra.Command{ | |||
Use: "eventbridge", | |||
Args: cobra.NoArgs, | |||
Args: cobra.MaximumNArgs(1), |
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 is a bug where we weren't accepting any log stream id for the update command. We fix this here and in other update commands down below.
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.
Good work here improving the test coverage, just pointed out a couple of non-blocking nits.
internal/cli/log_streams_datadog.go
Outdated
@@ -132,6 +132,16 @@ func updateLogStreamsDatadogCmd(cli *cli) *cobra.Command { | |||
return fmt.Errorf("failed to read log stream with ID %s: %w", inputs.ID, err) | |||
} | |||
|
|||
if oldLogStream.GetType() != string(logStreamTypeDatadog) { | |||
return fmt.Errorf( | |||
"the log stream with ID %q is of type %q instead of datadog, "+ |
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.
Maybe use logStreamTypeDatadog
here?
@@ -124,6 +124,16 @@ func updateLogStreamsAmazonEventBridgeCmd(cli *cli) *cobra.Command { | |||
return fmt.Errorf("failed to read log stream with ID %s: %w", inputs.ID, err) | |||
} | |||
|
|||
if oldLogStream.GetType() != string(logStreamTypeAmazonEventBridge) { | |||
return fmt.Errorf( | |||
"the log stream with ID %q is of type %q instead of eventbridge, "+ |
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.
Maybe use logStreamTypeAmazonEventBridge
here?
@@ -139,6 +139,16 @@ func updateLogStreamsAzureEventGridCmd(cli *cli) *cobra.Command { | |||
return fmt.Errorf("failed to read log stream with ID %s: %w", inputs.ID, err) | |||
} | |||
|
|||
if oldLogStream.GetType() != string(logStreamTypeAzureEventGrid) { | |||
return fmt.Errorf( | |||
"the log stream with ID %q is of type %q instead of eventgrid, "+ |
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.
Maybe use logStreamTypeAzureEventGrid
here?
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.
Same for the other log types
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.
Fixed in 129d72f
(#663)
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 work 👍
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #663 +/- ##
=======================================
Coverage ? 51.26%
=======================================
Files ? 92
Lines ? 11576
Branches ? 0
=======================================
Hits ? 5934
Misses ? 5213
Partials ? 429
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
🔧 Changes
Expands the test cases for the logs command and its relative subcommands and also fixes raised bugs. More in the comments down below.
📚 References
🔬 Testing
📝 Checklist