Skip to content

Update services.go, add MSSQL database connection to services.go#20

Open
ronydahdal wants to merge 13 commits intomainfrom
ronydahdal/sql-migration
Open

Update services.go, add MSSQL database connection to services.go#20
ronydahdal wants to merge 13 commits intomainfrom
ronydahdal/sql-migration

Conversation

@ronydahdal
Copy link

No description provided.

@ronydahdal ronydahdal requested a review from jumarmartin April 11, 2023 00:34
@jumarmartin
Copy link
Member

I'm getting a couple errors in my VSCode, could you check this out?:

	"resource": "/Users/jumar/src/elonsoc/ods/backend/main.go",
	"owner": "_generated_diagnostic_collection_name_#1",
	"code": {
		"value": "WrongArgCount",
		"target": {
			"$mid": 1,
			"external": "https://pkg.go.dev/golang.org/x/tools/internal/typesinternal#WrongArgCount",
			"path": "/golang.org/x/tools/internal/typesinternal",
			"scheme": "https",
			"authority": "pkg.go.dev",
			"fragment": "WrongArgCount"
		}
	},
	"severity": 8,
	"message": "not enough arguments in call to service.NewService\n\thave (string, string, string)\n\twant (string, string, string, string)",
	"source": "compiler",
	"startLineNumber": 128,
	"startColumn": 67,
	"endLineNumber": 128,
	"endColumn": 67
},{
	"resource": "/Users/jumar/src/elonsoc/ods/backend/main.go",
	"owner": "_generated_diagnostic_collection_name_#1",
	"code": {
		"value": "MissingFieldOrMethod",
		"target": {
			"$mid": 1,
			"path": "/golang.org/x/tools/internal/typesinternal",
			"scheme": "https",
			"authority": "pkg.go.dev",
			"fragment": "MissingFieldOrMethod"
		}
	},
	"severity": 8,
	"message": "Services.Db undefined (type *service.Service has no field or method Db)",
	"source": "compiler",
	"startLineNumber": 258,
	"startColumn": 13,
	"endLineNumber": 258,
	"endColumn": 15
},{
	"resource": "/Users/jumar/src/elonsoc/ods/backend/service/services.go",
	"owner": "_generated_diagnostic_collection_name_#1",
	"code": {
		"value": "UndeclaredImportedName",
		"target": {
			"$mid": 1,
			"external": "https://pkg.go.dev/golang.org/x/tools/internal/typesinternal#UndeclaredImportedName",
			"path": "/golang.org/x/tools/internal/typesinternal",
			"scheme": "https",
			"authority": "pkg.go.dev",
			"fragment": "UndeclaredImportedName"
		}
	},
	"severity": 8,
	"message": "DB not declared by package mssql",
	"source": "compiler",
	"startLineNumber": 31,
	"startColumn": 16,
	"endLineNumber": 31,
	"endColumn": 18
},{
	"resource": "/Users/jumar/src/elonsoc/ods/backend/service/services.go",
	"owner": "_generated_diagnostic_collection_name_#1",
	"code": {
		"value": "UndeclaredImportedName",
		"target": {
			"$mid": 1,
			"path": "/golang.org/x/tools/internal/typesinternal",
			"scheme": "https",
			"authority": "pkg.go.dev",
			"fragment": "UndeclaredImportedName"
		}
	},
	"severity": 8,
	"message": "DB not declared by package mssql",
	"source": "compiler",
	"startLineNumber": 80,
	"startColumn": 58,
	"endLineNumber": 80,
	"endColumn": 60
},{
	"resource": "/Users/jumar/src/elonsoc/ods/backend/service/services.go",
	"owner": "_generated_diagnostic_collection_name_#1",
	"code": {
		"value": "UndeclaredImportedName",
		"target": {
			"$mid": 1,
			"path": "/golang.org/x/tools/internal/typesinternal",
			"scheme": "https",
			"authority": "pkg.go.dev",
			"fragment": "UndeclaredImportedName"
		}
	},
	"severity": 8,
	"message": "Connect not declared by package mssql",
	"source": "compiler",
	"startLineNumber": 83,
	"startColumn": 27,
	"endLineNumber": 83,
	"endColumn": 34
},{
	"resource": "/Users/jumar/src/elonsoc/ods/backend/go.mod",
	"owner": "_generated_diagnostic_collection_name_#1",
	"severity": 4,
	"message": "github.com/jackc/pgx is not used in this module",
	"source": "go mod tidy",
	"startLineNumber": 24,
	"startColumn": 2,
	"endLineNumber": 24,
	"endColumn": 42
},{
	"resource": "/Users/jumar/src/elonsoc/ods/backend/go.mod",
	"owner": "_generated_diagnostic_collection_name_#1",
	"severity": 4,
	"message": "github.com/pkg/errors is not used in this module",
	"source": "go mod tidy",
	"startLineNumber": 25,
	"startColumn": 2,
	"endLineNumber": 25,
	"endColumn": 30
}]```

Copy link
Member

@jumarmartin jumarmartin left a comment

Choose a reason for hiding this comment

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

please look at the comment previous to this!

@ronydahdal
Copy link
Author

Seems like Main.go's paramaters haven't been updated to account for the new mssql connection, will push a commit to update main.go

Backbones of ODS's migration from Elon's MSSQL Database into the ODS PostgreSQL Database
Copy link
Member

@jumarmartin jumarmartin left a comment

Choose a reason for hiding this comment

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

Great second look! I added some comments that should help steer you in the right direction.

Some logistical notes: try to also update the docker-compse.yaml which should help with testing!

I do have another branch that has the changes included if you'd like me to commit that.

// empty _ because Exec returns both a sql result obj & err value
// we're evaluating err value, hence the _
_, err = pgConn.Exec(`
CREATE TABLE elonbuildingspg (
Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't concern our code with the creation of tables, just the connection itself and possibly pulling from it?


for mssqlRows.Next() {
// declaration of variables to hold values of each column in row
var buildings_id string
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this into a struct? It looks like we reuse this in the future.

Copy link
Author

Choose a reason for hiding this comment

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

you're right, makes most sense to have it as a struct rather than keep redefining it, we can also use this struct in the json encoder file.

"context"


"github.com/denisenkom/go-mssqldb"
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
"github.com/denisenkom/go-mssqldb"
mssql "github.com/denisenkom/go-mssqldb"

Logger *logrus.Logger
Db *pgx.Conn
PgDb *pgx.Conn
MsDb *mssql.DB
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
MsDb *mssql.DB
PgDb *pgx.Conn

@@ -15,7 +15,8 @@ package service

import (
"context"
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
"context"
"database/sql/driver"

return connection
}

func InitMsDB(msdbURL string, log *logrus.Logger) *mssql.DB {
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
func InitMsDB(msdbURL string, log *logrus.Logger) *mssql.DB {
func InitMsDB(msdbURL string, log *logrus.Logger) *driver.Conn {

backend/main.go Outdated
// get our pertinent information from the environment variables or the command line
servicePort := flag.String("port", os.Getenv("PORT"), "port to run server on")
databaseURL := flag.String("database_url", os.Getenv("DATABASE_URL"), "database url")
postgresURL := flag.String("postgres_url", os.Getenv("POSTGRES_URL"), "postgres url")
Copy link
Member

Choose a reason for hiding this comment

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

Our database, in the future, could be anything! In future PRs we will abstract away from using pgx in initializing the database and instead just require whatever database someone uses to conform an interface.

Suggested change
postgresURL := flag.String("postgres_url", os.Getenv("POSTGRES_URL"), "postgres url")
databaseURL := flag.String("database_url", os.Getenv("DATABASE_URL"), "database url")

backend/main.go Outdated
Comment on lines +256 to +257
if *postgresURL == "" {
log.Fatal("postgres url not set")
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
if *postgresURL == "" {
log.Fatal("postgres url not set")
if *databaseURL == "" {
log.Fatal("database url not set")

Comment on lines +11 to +12

func migrate() {
Copy link
Member

Choose a reason for hiding this comment

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

Lets add a todo here.

Suggested change
func migrate() {
//TODO(@ronydahdal) finish :)
func migrate() {

Copy link
Member

Choose a reason for hiding this comment

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

I recommend moving this to a folder that might be more suited for a one-off run.

@jumarmartin
Copy link
Member

It looks like Microsoft has forked the currently used mssql integration: https://github.com/microsoft/go-mssqldb.

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