Skip to content
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 MySQL data store driver implementation #841

Merged
merged 9 commits into from
Jun 26, 2023

Conversation

NeerajGartia21
Copy link
Collaborator

Description of your changes

Fixes #

I have:

  • Read and followed KubeVela's contribution process.
  • Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
  • Run yarn lint to ensure the frontend changes are ready for review.
  • Run make reviewableto ensure the server changes are ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

Special notes for your reviewer

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch coverage: 2.71% and project coverage change: -1.47 ⚠️

Comparison is base (ff8382c) 60.46% compared to head (8b9e808) 59.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #841      +/-   ##
==========================================
- Coverage   60.46%   59.00%   -1.47%     
==========================================
  Files         113      114       +1     
  Lines       19607    19818     +211     
==========================================
- Hits        11856    11694     -162     
- Misses       6299     6687     +388     
+ Partials     1452     1437      -15     
Flag Coverage Δ
apiserver-unittests 32.15% <2.89%> (-1.70%) ⬇️
server-e2e-tests 48.43% <0.00%> (-0.56%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/server/domain/model/application.go 86.92% <ø> (ø)
pkg/server/domain/model/cluster.go 100.00% <ø> (ø)
pkg/server/domain/model/env.go 100.00% <ø> (ø)
pkg/server/domain/model/envbinding.go 100.00% <ø> (ø)
pkg/server/domain/model/pipeline.go 100.00% <ø> (ø)
pkg/server/domain/model/plugin.go 100.00% <ø> (ø)
pkg/server/domain/model/project.go 95.45% <ø> (ø)
pkg/server/domain/model/system_info.go 100.00% <ø> (ø)
pkg/server/domain/model/target.go 100.00% <ø> (ø)
pkg/server/domain/model/user.go 79.79% <ø> (ø)
... and 3 more

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@chivalryq chivalryq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR's content is not enough to be merged. We can attach the changes with the implementation of MySQL driver.

@NeerajGartia21
Copy link
Collaborator Author

Sure! I will add the implementation of the MySql driver methods ASAP.

pkg/server/infrastructure/datastore/mysql/mysql.go Outdated Show resolved Hide resolved

func TestMySQL(t *testing.T) {
_, err := New(context.TODO(), datastore.Config{
URL: "root:kubevelaSQL123@tcp(127.0.0.1:3306)/kubevela?parseTime=True",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better we add the format explaination in our document. Or in helm chart.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format explanation for the Database URL right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it can be added while adding documentation for mysql driver.

pkg/server/infrastructure/datastore/mysql/mysql.go Outdated Show resolved Hide resolved
pkg/server/infrastructure/datastore/mysql/mysql.go Outdated Show resolved Hide resolved
pkg/server/infrastructure/datastore/mysql/mysql_test.go Outdated Show resolved Hide resolved
@chivalryq chivalryq changed the title Feat: add tags to make models compatible for mysql Feat: add MySQL data store driver implementation Jun 16, 2023
@NeerajGartia21 NeerajGartia21 force-pushed the mysql-driver branch 4 times, most recently from 2cebb92 to bf81ad5 Compare June 21, 2023 02:49
Copy link
Member

@chivalryq chivalryq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very close. Now we have some details to deal with.

pkg/server/infrastructure/datastore/mysql/mysql.go Outdated Show resolved Hide resolved
pkg/server/infrastructure/datastore/mysql/mysql.go Outdated Show resolved Hide resolved
pkg/server/infrastructure/datastore/mysql/mysql.go Outdated Show resolved Hide resolved
pkg/server/infrastructure/datastore/mysql/mysql.go Outdated Show resolved Hide resolved
Signed-off-by: Neeraj Gartia <[email protected]>
Signed-off-by: Neeraj Gartia <[email protected]>
Copy link
Member

@chivalryq chivalryq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great job!

@NeerajGartia21
Copy link
Collaborator Author

Thanks @chivalryq !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants