-
Notifications
You must be signed in to change notification settings - Fork 14
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: CloudDedicated database creation ignores the given name #98
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
- Coverage 85.01% 79.21% -5.80%
==========================================
Files 14 14
Lines 1121 1121
==========================================
- Hits 953 888 -65
- Misses 139 203 +64
- Partials 29 30 +1 ☔ View full report in Codecov by Sentry. |
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 your PR! 👍 Before we can proceed with accepting it, there are a few requirements that need to be met:
if db.ClusterDatabaseName == "" { | ||
if d.client.config.Database == "" { | ||
return errors.New("database name must not be empty") | ||
} | ||
db.ClusterDatabaseName = d.client.config.Database |
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.
Describe this behaviour in function documentation.
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.
Sure, but before doing it, I'm considering whether this fallback may be unexpected for users. Do you think this behavior should be left with documentation, or should we just drop it?
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.
Keep this fallback and update documentation.
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.
Addressed with eea7b21
eea7b21
to
f72bdfd
Compare
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.
Thanks again for your PR 👍
LGTM 🚀
Proposed Changes
Cloud Dedicated database creation ignores the database name given by an argument, but applies the database name within client configuration. This behavior differs from the API spec and user intuitions, so I'd say it's a bug.
Checklist