-
Notifications
You must be signed in to change notification settings - Fork 109
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
Feat: Add support for OpenGauss and Postgres database types #865
Conversation
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 for contributing. Please fix the CI.
@@ -31,6 +32,7 @@ import ( | |||
"go.mongodb.org/mongo-driver/mongo" | |||
"go.mongodb.org/mongo-driver/mongo/options" | |||
mysqlgorm "gorm.io/driver/mysql" | |||
opengaussgorm "gorm.io/driver/postgres" |
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.
Postgres will be later supported. I think we should leave the import alias as postgresgorm.
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.
ok let me modify it
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #865 +/- ##
===========================================
- Coverage 58.94% 47.69% -11.25%
===========================================
Files 114 115 +1
Lines 19801 20032 +231
===========================================
- Hits 11671 9554 -2117
- Misses 6694 9229 +2535
+ Partials 1436 1249 -187
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
8fbe67d
to
4363e88
Compare
4b3c8f8
to
52ccd88
Compare
Thanks for your contribution @lgj101 . This PR looks good to me !! CC: @chivalryq |
@@ -72,6 +72,16 @@ jobs: | |||
mysql database: 'kubevela' | |||
mysql root password: 'kubevelaSQL123' | |||
|
|||
- name: Set up OpenGauss | |||
uses: lgj101/[email protected] |
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.
Is this really setup a OpenGauss DB? I checked Harmon758/postgresql-action@master...lgj101:opengauss-action:master and it seems to change the exposed variable only. If this is only changes I think we can just uses the origin action and implement the Postgres driver.
After all if we setup a Postgres here we can only checked the compatibility with Postgres. We can't say VelaUX "supports OpenGauss" because we don't have test.
However if we implement the Postgres driver and we can add notes in document that because OpenGauss is compatible with Postgres, you can also use OpenGauss.
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.
I'm sorry, yes this action setup a opengauss db. Given the popularity of postgres and the compatibility between them, we can just rename the driver/opengauss
to driver/postgresql
and keep the openguass test.
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.
Okay, I have already made the modifications like this
Signed-off-by: ligjn <[email protected]> delete comment Signed-off-by: ligjn <[email protected]> bug fix Signed-off-by: ligjn <[email protected]> modify package name Signed-off-by: ligjn <[email protected]> modify package name Signed-off-by: ligjn <[email protected]> opengauss ci setup Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit ci Signed-off-by: ligjn <[email protected]> edit log level Signed-off-by: ligjn <[email protected]> change opengauss to postgres Signed-off-by: ligjn <[email protected]> edit ci change opengauss to postgres
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
Description of your changes
Add support for OpenGauss database types
Fixes #
I have:
yarn lint
to ensure the frontend changes are ready for review.make reviewable
to ensure the server changes are ready for review.backport release-x.y
labels to auto-backport this PR if necessary.