You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/10/18 11:52:59 UTC
[GitHub] [incubator-devlake] mappjzc opened a new pull request, #3473: refactor: refactor migrationscripts
mappjzc opened a new pull request, #3473:
URL: https://github.com/apache/incubator-devlake/pull/3473
# Summary
refactor migrationscripts for gitlab
Add AddTablerColumn and DropTablerColumn for dal
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes.
Please fill out as many sections below as possible.
-->
### Does this close any open issues?
Closes #3472
### Screenshots
Include any relevant screenshots here.
### Other Information
Any other information that is important to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on a diff in pull request #3473: refactor: refactor migrationscripts
Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #3473:
URL: https://github.com/apache/incubator-devlake/pull/3473#discussion_r999069174
##########
impl/dalgorm/dalgorm.go:
##########
@@ -190,6 +190,21 @@ func (d *Dalgorm) AddColumn(table, columnName, columnType string) errors.Error {
return d.Exec("ALTER TABLE ? ADD ? ?", clause.Table{Name: table}, clause.Column{Name: columnName}, clause.Expr{SQL: columnType})
}
+// AddTablerColumn add colume with Tabler args
+func (d *Dalgorm) AddTablerColumn(dst dal.Tabler, field string) errors.Error {
Review Comment:
Can't we use `AutoMigrate`?
##########
plugins/gitlab/models/migrationscripts/20220906_fix_duration_to_float8.go:
##########
@@ -40,40 +40,46 @@ func (GitlabJob20220906) TableName() string {
return "_tool_gitlab_jobs"
}
-func (*fixDurationToFloat8) Up(ctx context.Context, db *gorm.DB) errors.Error {
- err := db.Migrator().AddColumn(&GitlabJob20220906{}, `duration2`)
+func (*fixDurationToFloat8) Up(baseRes core.BasicRes) errors.Error {
+ db := baseRes.GetDal()
+ err := db.AddTablerColumn(&GitlabJob20220906{}, `duration2`)
Review Comment:
FYI https://github.com/apache/incubator-devlake/blob/main/models/migrationscripts/20220929_change_leadtimeminutes_to_int64.go
##########
plugins/gitlab/models/migrationscripts/20220729_modify_gilab_ci.go:
##########
@@ -77,15 +78,16 @@ func (GitlabJob20220729) TableName() string {
return "_tool_gitlab_jobs"
}
-func (*modifyGitlabCI) Up(ctx context.Context, db *gorm.DB) errors.Error {
- err := db.Migrator().AddColumn(&GitlabPipeline20220729{}, "gitlab_updated_at")
+func (*modifyGitlabCI) Up(baseRes core.BasicRes) errors.Error {
+ db := baseRes.GetDal()
+ err := db.AddTablerColumn(&GitlabPipeline20220729{}, "gitlab_updated_at")
Review Comment:
`model struct` must be camelCase: GitlabPipeline20220729 -> gitlabPipeline20220729
##########
impl/dalgorm/dalgorm.go:
##########
@@ -207,6 +222,21 @@ func (d *Dalgorm) DropColumns(table string, columnNames ...string) errors.Error
return nil
}
+// DropTablerColumn drop colume with Tabler args
+func (d *Dalgorm) DropTablerColumn(dst dal.Tabler, field string) errors.Error {
Review Comment:
What is the necessity of it? Can't we use `DropColumns`
##########
plugins/gitlab/models/migrationscripts/20220729_modify_gilab_ci.go:
##########
@@ -18,12 +18,13 @@ limitations under the License.
package migrationscripts
import (
- "context"
- "github.com/apache/incubator-devlake/errors"
"time"
+ "github.com/apache/incubator-devlake/errors"
+ "github.com/apache/incubator-devlake/helpers/migrationhelper"
+ "github.com/apache/incubator-devlake/plugins/core"
+
"github.com/apache/incubator-devlake/models/migrationscripts/archived"
- "gorm.io/gorm"
)
type modifyGitlabCI struct{}
Review Comment:
Please rename the name of the script to a more descriptive one
##########
plugins/gitlab/models/migrationscripts/20220906_fix_duration_to_float8.go:
##########
@@ -40,40 +40,46 @@ func (GitlabJob20220906) TableName() string {
return "_tool_gitlab_jobs"
}
-func (*fixDurationToFloat8) Up(ctx context.Context, db *gorm.DB) errors.Error {
- err := db.Migrator().AddColumn(&GitlabJob20220906{}, `duration2`)
+func (*fixDurationToFloat8) Up(baseRes core.BasicRes) errors.Error {
+ db := baseRes.GetDal()
+ err := db.AddTablerColumn(&GitlabJob20220906{}, `duration2`)
Review Comment:
or use `migrationhelper.Transform`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on a diff in pull request #3473: refactor: refactor migrationscripts
Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #3473:
URL: https://github.com/apache/incubator-devlake/pull/3473#discussion_r1003284378
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,138 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change the type of specified columns for the table
+func ChangeColumnsType[D any](
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ tableName string,
+ columns []string,
+ update func(tmpColumnParams []interface{}) errors.Error,
+) (err errors.Error) {
+ db := basicRes.GetDal()
+ tmpColumnsNames := make([]string, len(columns))
+ for i, v := range columns {
+ tmpColumnsNames[i] = fmt.Sprintf("%s_%s", v, hashScript(script))
+ err = db.RenameColumn(tableName, v, tmpColumnsNames[i])
+ if err != nil {
+ return err
+ }
+
+ defer func(tmpColumnName string, ColumnsName string) {
+ if err != nil {
+ err1 := db.RenameColumn(tableName, tmpColumnName, ColumnsName)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by RenameColum failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }(tmpColumnsNames[i], v)
+ }
+
+ err = db.AutoMigrate(new(D), dal.From(tableName))
+ if err != nil {
+ return errors.Default.Wrap(err, "AutoMigrate for Add Colume Error")
+ }
+
+ defer func() {
+ if err != nil {
+ err1 := db.DropColumns(tableName, columns...)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by DropColume failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }()
+
+ if update == nil {
+ params := make([]interface{}, 0, len(columns)*2+1)
+ params = append(params, clause.Table{Name: tableName})
+ updateStr := "UPDATE ? SET "
+ for i, v := range columns {
+ if i > 0 {
+ updateStr += " AND "
+ }
+ updateStr += "? = ?"
+ params = append(params, clause.Column{Name: v})
+ params = append(params, clause.Column{Name: tmpColumnsNames[i]})
+ }
+ err = db.Exec(updateStr, params...)
+ } else {
+ params := make([]interface{}, 0, len(tmpColumnsNames))
+ for _, v := range tmpColumnsNames {
+ params = append(params, clause.Column{Name: v})
+ }
+ err = update(params)
+ }
+ if err != nil {
+ return err
+ }
+
+ err = db.DropColumns(tableName, tmpColumnsNames...)
+ if err != nil {
+ return err
+ }
+
+ return nil
+}
+
+// TransformColumns change the type of specified columns for the table and transform data one by one
+func TransformColumns[S any, D any](
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ tableName string,
+ columns []string,
+ transform func(src *S) (*D, errors.Error),
+) (err errors.Error) {
+ db := basicRes.GetDal()
+ return ChangeColumnsType[D](
+ basicRes,
+ script,
+ tableName,
+ columns,
+ func(tmpColumnParams []interface{}) errors.Error {
+ // create selectStr for transform tmpColumnsNames
+ params := make([]interface{}, 0, len(columns)*2+1)
+ selectStr := "SELECT * "
+ for i, v := range columns {
+ selectStr += ",? as ?"
+ params = append(params, tmpColumnParams[i])
+ params = append(params, clause.Column{Name: v})
+ }
+ selectStr += " FROM ? "
+ params = append(params, clause.Table{Name: tableName})
+
+ cursor, err := db.RawCursor(selectStr, params...)
Review Comment:
I think we could use `db.Cursor` rather than `db.RawCursor`.
It would be better to use `db.Cursor` because it is strong-typed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on a diff in pull request #3473: refactor: refactor migrationscripts
Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #3473:
URL: https://github.com/apache/incubator-devlake/pull/3473#discussion_r1002971755
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,134 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change some Columns type for one table
+func ChangeColumnsType(
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ newTable core.Tabler,
+ Columns []string,
Review Comment:
Why is it PascalCased?
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,134 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change some Columns type for one table
Review Comment:
change **the type of specified columns for the table**
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,134 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change some Columns type for one table
+func ChangeColumnsType(
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ newTable core.Tabler,
+ Columns []string,
+ update func(tmpColumnsNames []string) errors.Error,
+) (err errors.Error) {
+ db := basicRes.GetDal()
+ tmpColumnsNames := make([]string, len(Columns))
+ for i, v := range Columns {
+ tmpColumnsNames[i] = fmt.Sprintf("%s_%s", v, hashScript(script))
+ err = db.RenameColumn(newTable.TableName(), v, tmpColumnsNames[i])
+ if err != nil {
+ return err
+ }
+
+ defer func(tmpColumnName string, ColumnsName string) {
+ if err != nil {
+ err1 := db.RenameColumn(newTable.TableName(), tmpColumnName, ColumnsName)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by RenameColum failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }(tmpColumnsNames[i], v)
+ }
+
+ err = db.AutoMigrate(newTable)
+ if err != nil {
+ return errors.Default.Wrap(err, "AutoMigrate for Add Colume Error")
+ }
+
+ defer func() {
+ if err != nil {
+ err1 := db.DropColumns(newTable.TableName(), Columns...)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by DropColume failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }()
+
+ if update == nil {
+ params := make([]interface{}, 0, len(Columns)*2+1)
+ params = append(params, clause.Table{Name: newTable.TableName()})
+ updateStr := "UPDATE ? SET "
+ for i, v := range Columns {
+ if i > 0 {
+ updateStr += " AND "
+ }
+ updateStr += "? = ?"
+ params = append(params, clause.Column{Name: v})
+ params = append(params, clause.Column{Name: tmpColumnsNames[i]})
+ }
+ err = db.Exec(updateStr, params...)
+ } else {
+ err = update(tmpColumnsNames)
+ }
+ if err != nil {
+ return err
+ }
+
+ err = db.DropColumns(newTable.TableName(), tmpColumnsNames...)
+ if err != nil {
+ return err
+ }
+
+ return nil
+}
+
+// ChangeColumnsTypeOneByOne change some Columns type for one table
+func ChangeColumnsTypeOneByOne[S any, D core.Tabler](
Review Comment:
How about `TransformColumns` because it acts much like the `TransformTable`
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,134 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change some Columns type for one table
+func ChangeColumnsType(
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ newTable core.Tabler,
+ Columns []string,
+ update func(tmpColumnsNames []string) errors.Error,
+) (err errors.Error) {
+ db := basicRes.GetDal()
+ tmpColumnsNames := make([]string, len(Columns))
+ for i, v := range Columns {
+ tmpColumnsNames[i] = fmt.Sprintf("%s_%s", v, hashScript(script))
+ err = db.RenameColumn(newTable.TableName(), v, tmpColumnsNames[i])
+ if err != nil {
+ return err
+ }
+
+ defer func(tmpColumnName string, ColumnsName string) {
+ if err != nil {
+ err1 := db.RenameColumn(newTable.TableName(), tmpColumnName, ColumnsName)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by RenameColum failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }(tmpColumnsNames[i], v)
+ }
+
+ err = db.AutoMigrate(newTable)
+ if err != nil {
+ return errors.Default.Wrap(err, "AutoMigrate for Add Colume Error")
+ }
+
+ defer func() {
+ if err != nil {
+ err1 := db.DropColumns(newTable.TableName(), Columns...)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by DropColume failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }()
+
+ if update == nil {
+ params := make([]interface{}, 0, len(Columns)*2+1)
+ params = append(params, clause.Table{Name: newTable.TableName()})
+ updateStr := "UPDATE ? SET "
+ for i, v := range Columns {
+ if i > 0 {
+ updateStr += " AND "
+ }
+ updateStr += "? = ?"
+ params = append(params, clause.Column{Name: v})
+ params = append(params, clause.Column{Name: tmpColumnsNames[i]})
+ }
+ err = db.Exec(updateStr, params...)
+ } else {
+ err = update(tmpColumnsNames)
+ }
+ if err != nil {
+ return err
+ }
+
+ err = db.DropColumns(newTable.TableName(), tmpColumnsNames...)
+ if err != nil {
+ return err
+ }
+
+ return nil
+}
+
+// ChangeColumnsTypeOneByOne change some Columns type for one table
+func ChangeColumnsTypeOneByOne[S any, D core.Tabler](
Review Comment:
Is `core.Tabler` a must? Can we ask for the `tableName` and `any` struct just like the `TransformTable` method? It would make much sense because the `tableName` is never change.
With current construct, the Developer would be so confuse to understand which struct should implement the `Tabler` interface.
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,134 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change some Columns type for one table
+func ChangeColumnsType(
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ newTable core.Tabler,
+ Columns []string,
+ update func(tmpColumnsNames []string) errors.Error,
+) (err errors.Error) {
+ db := basicRes.GetDal()
+ tmpColumnsNames := make([]string, len(Columns))
+ for i, v := range Columns {
+ tmpColumnsNames[i] = fmt.Sprintf("%s_%s", v, hashScript(script))
+ err = db.RenameColumn(newTable.TableName(), v, tmpColumnsNames[i])
+ if err != nil {
+ return err
+ }
+
+ defer func(tmpColumnName string, ColumnsName string) {
+ if err != nil {
+ err1 := db.RenameColumn(newTable.TableName(), tmpColumnName, ColumnsName)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by RenameColum failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }(tmpColumnsNames[i], v)
+ }
+
+ err = db.AutoMigrate(newTable)
+ if err != nil {
+ return errors.Default.Wrap(err, "AutoMigrate for Add Colume Error")
+ }
+
+ defer func() {
+ if err != nil {
+ err1 := db.DropColumns(newTable.TableName(), Columns...)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by DropColume failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }()
+
+ if update == nil {
+ params := make([]interface{}, 0, len(Columns)*2+1)
+ params = append(params, clause.Table{Name: newTable.TableName()})
+ updateStr := "UPDATE ? SET "
+ for i, v := range Columns {
+ if i > 0 {
+ updateStr += " AND "
+ }
+ updateStr += "? = ?"
+ params = append(params, clause.Column{Name: v})
+ params = append(params, clause.Column{Name: tmpColumnsNames[i]})
+ }
+ err = db.Exec(updateStr, params...)
+ } else {
+ err = update(tmpColumnsNames)
+ }
+ if err != nil {
+ return err
+ }
+
+ err = db.DropColumns(newTable.TableName(), tmpColumnsNames...)
+ if err != nil {
+ return err
+ }
+
+ return nil
+}
+
+// ChangeColumnsTypeOneByOne change some Columns type for one table
+func ChangeColumnsTypeOneByOne[S any, D core.Tabler](
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ Columns []string,
+ update func(src *S) (D, errors.Error),
Review Comment:
`update` is not a good name here, consider `transform`
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,134 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change some Columns type for one table
+func ChangeColumnsType(
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ newTable core.Tabler,
Review Comment:
newTable?
##########
helpers/migrationhelper/migrationhelper.go:
##########
@@ -42,6 +43,134 @@ func AutoMigrateTables(basicRes core.BasicRes, dst ...interface{}) errors.Error
return nil
}
+// ChangeColumnsType change some Columns type for one table
+func ChangeColumnsType(
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ newTable core.Tabler,
+ Columns []string,
+ update func(tmpColumnsNames []string) errors.Error,
+) (err errors.Error) {
+ db := basicRes.GetDal()
+ tmpColumnsNames := make([]string, len(Columns))
+ for i, v := range Columns {
+ tmpColumnsNames[i] = fmt.Sprintf("%s_%s", v, hashScript(script))
+ err = db.RenameColumn(newTable.TableName(), v, tmpColumnsNames[i])
+ if err != nil {
+ return err
+ }
+
+ defer func(tmpColumnName string, ColumnsName string) {
+ if err != nil {
+ err1 := db.RenameColumn(newTable.TableName(), tmpColumnName, ColumnsName)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by RenameColum failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }(tmpColumnsNames[i], v)
+ }
+
+ err = db.AutoMigrate(newTable)
+ if err != nil {
+ return errors.Default.Wrap(err, "AutoMigrate for Add Colume Error")
+ }
+
+ defer func() {
+ if err != nil {
+ err1 := db.DropColumns(newTable.TableName(), Columns...)
+ if err1 != nil {
+ err = errors.Default.Wrap(err, fmt.Sprintf("RollBack by DropColume failed.Relevant data needs to be repaired manually.%s", err1.Error()))
+ }
+ }
+ }()
+
+ if update == nil {
+ params := make([]interface{}, 0, len(Columns)*2+1)
+ params = append(params, clause.Table{Name: newTable.TableName()})
+ updateStr := "UPDATE ? SET "
+ for i, v := range Columns {
+ if i > 0 {
+ updateStr += " AND "
+ }
+ updateStr += "? = ?"
+ params = append(params, clause.Column{Name: v})
+ params = append(params, clause.Column{Name: tmpColumnsNames[i]})
+ }
+ err = db.Exec(updateStr, params...)
+ } else {
+ err = update(tmpColumnsNames)
+ }
+ if err != nil {
+ return err
+ }
+
+ err = db.DropColumns(newTable.TableName(), tmpColumnsNames...)
+ if err != nil {
+ return err
+ }
+
+ return nil
+}
+
+// ChangeColumnsTypeOneByOne change some Columns type for one table
+func ChangeColumnsTypeOneByOne[S any, D core.Tabler](
+ basicRes core.BasicRes,
+ script core.MigrationScript,
+ Columns []string,
Review Comment:
Same as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh merged pull request #3473: refactor: refactor migrationscripts
Posted by GitBox <gi...@apache.org>.
klesh merged PR #3473:
URL: https://github.com/apache/incubator-devlake/pull/3473
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org