-
Notifications
You must be signed in to change notification settings - Fork 178
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
chore: Improves error handling messages in advanced cluster tpf #2914
Conversation
c55d4ad
to
fa993d2
Compare
Only |
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.
LGTM
const ( | ||
errorReadDatasource = "Error reading advanced cluster datasource" | ||
errorReadDatasourceAsymmetric = "Error reading advanced cluster datasource" | ||
errorReadDatasourceAsymmetricDetail = "Cluster name %s. Please add `use_replication_spec_per_shard = true` to your data source configuration to enable asymmetric shard support. Refer to documentation for more details." |
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.
Could you provide a link to the doc as part of the error message? We do this in the CLI. You should ask the doc team for a dochub link if the documentation is in the MDB website
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 point. Added the link to our migration guide that has more details in 1c35df4
@@ -14,6 +14,12 @@ import ( | |||
var _ datasource.DataSource = &ds{} | |||
var _ datasource.DataSourceWithConfigure = &ds{} | |||
|
|||
const ( |
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.
Are these principles documented somewhere in the repo?
Error diagnostics principle:
Summary should be a real sentence, static, and unique string.
Detail can be more sentences with parameters.
Include the attribute name for deprecated fields
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.
Thank you for the comments 😄
Added this to our techincal discussion.
Description
Improves error handling messages in advanced cluster tpf
Error diagnostics principle:
Summary
should be a real sentence, static, and unique string.Detail
can be more sentences with parameters.Alo changes:
Link to any related issue(s): CLOUDP-288298
Type of change:
Required Checklist:
Further comments