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

Add mysql to mapper-framework for enhancing pushmethod db. #5376

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

luomengY
Copy link
Member

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add mysql to mapper-framework for enhancing pushmethod db.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@kubeedge-bot kubeedge-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 19, 2024
@kubeedge-bot kubeedge-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 19, 2024
@kubeedge-bot kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2024
@luomengY luomengY force-pushed the mysql branch 15 times, most recently from e80e0d7 to d2dbd92 Compare January 25, 2024 08:44
@luomengY luomengY closed this Jan 25, 2024
@luomengY luomengY reopened this Jan 25, 2024
@luomengY luomengY changed the title [wip]Add mysql to mapper-framework for enhancing pushmethod db. Add mysql to mapper-framework for enhancing pushmethod db. Jan 25, 2024
@kubeedge-bot kubeedge-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 25, 2024
@luomengY luomengY closed this Jan 25, 2024
@luomengY luomengY reopened this Jan 25, 2024
@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2024
@wbc6080
Copy link
Contributor

wbc6080 commented Feb 26, 2024

cc @cl2017

@luomengY
Copy link
Member Author

Add it to version v1.17.0. @Shelley-BaoYue

@Shelley-BaoYue Shelley-BaoYue added this to the v1.17 milestone Feb 29, 2024
@@ -0,0 +1,93 @@
package mysql
Copy link
Member

Choose a reason for hiding this comment

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

Add KubeEdge License header for all new files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ihave added it.

Comment on lines 43 to 44
github.com/kubeedge/kubeedge => ../../../../../../../../kubeedge
github.com/kubeedge/mapper-framework => ../../../mapper-framework
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
github.com/kubeedge/kubeedge => ../../../../../../../../kubeedge
github.com/kubeedge/mapper-framework => ../../../mapper-framework
github.com/kubeedge/kubeedge => ../../../../../../kubeedge
github.com/kubeedge/mapper-framework => ../mapper-framework

Considering the e2e test of mapper-framework, the directory where the generated mapper project is located should be used as the standard for dependent applications. Otherwise, the directory actually referenced by the generated mapper project is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Considering the e2e test of mapper-framework, the directory where the generated mapper project is located should be used as the standard for dependent applications. Otherwise, the directory actually referenced by the generated mapper project is incorrect.

ok

@kubeedge-bot kubeedge-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024
@wbc6080
Copy link
Contributor

wbc6080 commented Apr 17, 2024

Verify vendor CI need to be fixed :) @luomengY

@luomengY luomengY closed this Apr 17, 2024
@luomengY luomengY reopened this Apr 17, 2024
@luomengY luomengY force-pushed the mysql branch 2 times, most recently from c251421 to 91de952 Compare April 17, 2024 09:12
@luomengY
Copy link
Member Author

Verify vendor CI need to be fixed :) @luomengY

ok

Copy link
Contributor

@wbc6080 wbc6080 left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2024
@@ -0,0 +1,106 @@
/*
Copyright 2024 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Copyright 2024 The Kubernetes Authors.
Copyright 2024 The KubeEdge Authors.

KubeEdge, thanks :)

@kubeedge-bot kubeedge-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2024
@@ -251,6 +251,7 @@ message DBMethod{
DBMethodInfluxdb2 influxdb2 = 1;
DBMethodRedis redis = 2;
DBMethodTDEngine tdengine = 3;
DBMethodMySQL mysql=4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a space to keep the format consistent.

Suggested change
DBMethodMySQL mysql=4;
DBMethodMySQL mysql = 4;

@@ -303,6 +304,20 @@ message TDEngineClientConfig{
string dbname = 2;
}


message DBMethodMySQL{
MySQLClientConfig mysqlClientConfig=1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

if err != nil {
klog.Errorf("close mysql failed with err:%v", err)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove redundant blank lines.

@@ -0,0 +1,70 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

There need be a blank line between paragraphs in copyrights. You can refer to other files.

github.com/golang/protobuf v1.5.3 // indirect
github.com/gorilla/mux v1.8.0 // indirect
github.com/influxdata/influxdb-client-go/v2 v2.13.0
github.com/kubeedge/kubeedge v1.16.0-beta.0
github.com/kubeedge/mapper-framework v0.0.0-20240119021034-e7755b2f421c
github.com/kubeedge/kubeedge v0.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change the way the mapper gomod is referenced in this pr? Wasn't it involved in other databases before? @luomengY @wbc6080

Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the change is not caused by the addition of a new database, but the content of the mapper upgrade framework. Since part of the mapper code now needs to reference the content in the kubeedge/mapper-framework repo, when we modify the mapper code, the e2e test still uses the content in the old version of the kubeedge/mapper-framework repo, which will cause conflicts. So we change this part to a local reference @Shelley-BaoYue

Signed-off-by: luomengY <2938893385@qq.com>
Copy link
Collaborator

@Shelley-BaoYue Shelley-BaoYue left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2024
Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

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

/approve

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu, wbc6080

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2024
@kubeedge-bot kubeedge-bot merged commit 9de5264 into kubeedge:master Apr 20, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants