Return a different error message if --provisioning flag is not set #233
Conversation
WalkthroughThe changes involve refactoring error handling and control flow in the codebase related to provisioning. Hardcoded error messages have been replaced with a constant variable for better maintainability. Additionally, a new validation step has been introduced to check whether provisioning is enabled before executing certain operations. This ensures that users receive clearer error messages when attempting to provision resources without the necessary configuration. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
pkg/connectorbuilder/connectorbuilder.go (2)
Line range hint
540-566: Improved error handling and messaging in the Grant methodThe changes enhance the error reporting by using a standardized message (
connector.ProvisioningNotEnabledMsg) when provisioning is not enabled. This aligns well with the PR objectives to provide clearer error messages.Consider adding the error message to the log output on line 565 for consistency:
-l.Error(connector.ProvisioningNotEnabledMsg, zap.String("resource_type", rt)) +l.Error("error: grant failed", zap.Error(errors.New(connector.ProvisioningNotEnabledMsg)), zap.String("resource_type", rt))This change would make the error logging consistent with other error logs in the method and provide more context in the logs.
Line range hint
574-600: Improved error handling and messaging in the Revoke methodThe changes in the
Revokemethod mirror those in theGrantmethod, enhancing error reporting by using a standardized message (connector.ProvisioningNotEnabledMsg) when provisioning is not enabled. This is consistent with the PR objectives and improves the overall error handling in the connector.For consistency with the
Grantmethod and other error logs, consider modifying the error logging on line 598:-l.Error(connector.ProvisioningNotEnabledMsg, zap.String("resource_type", rt)) +l.Error("error: revoke failed", zap.Error(errors.New(connector.ProvisioningNotEnabledMsg)), zap.String("resource_type", rt))This change would provide more context in the logs and maintain consistency across error reporting in the file.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- internal/connector/noop_provisioner.go (1 hunks)
- pkg/cli/commands.go (3 hunks)
- pkg/connectorbuilder/connectorbuilder.go (5 hunks)
Additional comments not posted (10)
internal/connector/noop_provisioner.go (8)
13-14: LGTM: New constant improves clarity and maintainabilityThe new
ProvisioningNotEnabledMsgconstant effectively addresses the PR objectives by providing a clearer error message. It correctly suggests using the--provisioningflag, which aligns with the requirements. Using a constant also improves code maintainability by centralizing the error message.
16-16: LGTM: Improved error handling in Grant methodThe
Grantmethod now correctly uses the newProvisioningNotEnabledMsgconstant, which aligns with the PR objectives. The use ofstatus.Errorwithcodes.FailedPreconditionis appropriate for this scenario, providing a clear and consistent error message.
20-20: LGTM: Consistent error handling in Revoke methodThe
Revokemethod now uses theProvisioningNotEnabledMsgconstant, maintaining consistency with theGrantmethod and aligning with the PR objectives. The error handling approach remains appropriate and consistent.
24-24: LGTM: Consistent error handling in CreateResource methodThe
CreateResourcemethod now uses theProvisioningNotEnabledMsgconstant, maintaining consistency with other methods and aligning with the PR objectives. The error handling approach remains appropriate and consistent.
28-28: LGTM: Consistent error handling in DeleteResource methodThe
DeleteResourcemethod now uses theProvisioningNotEnabledMsgconstant, maintaining consistency with other methods and aligning with the PR objectives. The error handling approach remains appropriate and consistent.
32-32: LGTM: Consistent error handling in RotateCredential methodThe
RotateCredentialmethod now uses theProvisioningNotEnabledMsgconstant, maintaining consistency with other methods and aligning with the PR objectives. The error handling approach remains appropriate and consistent.
36-36: LGTM: Consistent error handling in CreateAccount methodThe
CreateAccountmethod now uses theProvisioningNotEnabledMsgconstant, maintaining consistency with other methods and aligning with the PR objectives. The error handling approach remains appropriate and consistent.
13-36: Summary: Excellent improvements in error handling and message clarityThe changes in this file effectively address the PR objectives and the linked issue (#114). By introducing the
ProvisioningNotEnabledMsgconstant and consistently using it across all methods of thenoopProvisionerstruct, the code now provides a clearer and more informative error message when provisioning is not enabled.Key improvements:
- Enhanced user experience with a more descriptive error message.
- Improved code maintainability by centralizing the error message in a constant.
- Consistent error handling across all methods.
These changes will help users understand the need for the
--provisioningflag more easily, potentially reducing confusion and support requests.pkg/cli/commands.go (2)
21-22: Approved: Necessary imports addedThe imports of
codesandstatusfromgoogle.golang.org/grpcare necessary for proper error handling using gRPC status codes.
70-70: Approved: VariableisProvisioningcorrectly captures certain provisioning actionsThe introduction of the
isProvisioningvariable appropriately checks if thegrant-entitlementorrevoke-grantflags are set, indicating provisioning actions.
| if isProvisioning && !v.GetBool("provisioning") { | ||
| return status.Error(codes.Unimplemented, connector.ProvisioningNotEnabledMsg) | ||
| } |
There was a problem hiding this comment.
Issue: isProvisioning does not account for all provisioning commands
The current condition for isProvisioning only checks the grant-entitlement and revoke-grant flags. However, other commands like create-account-login, delete-resource, and rotate-credentials also perform provisioning actions. This omission could lead to situations where users do not receive the intended error message when provisioning is disabled but attempt these actions.
Apply this diff to include all provisioning-related flags in the isProvisioning variable:
-isProvisioning := v.GetString("grant-entitlement") != "" || v.GetString("revoke-grant") != ""
+isProvisioning := v.GetString("grant-entitlement") != "" ||
+ v.GetString("revoke-grant") != "" ||
+ v.GetString("create-account-login") != "" ||
+ v.GetString("delete-resource") != "" ||
+ v.GetString("rotate-credentials") != ""Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Right now if you try to provision something without the --provisioning flag, we error with "resource type does not have provisioner configured". This error message should be something like "provisioning is not enabled. try running with --provisioning".
Fixes #114.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
GrantandRevokemethods for improved readability and maintainability.