-
Notifications
You must be signed in to change notification settings - Fork 3
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
[draft] HTTP server with connection to database #247
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Kevin H. Luu <[email protected]>
reefd/main.go
Outdated
|
||
// a function to connect to database given a path to .db file | ||
func connectToDatabase(dbPath string) (*sql.DB, error) { | ||
db, err := sql.Open("sqlite3", dbPath) |
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.
return sql.Open("sqlite3", dbPath)
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.
removed this func and used sql.Open("sqlite3", dbPath)
instead
reefd/main.go
Outdated
"encoding/json" | ||
) | ||
|
||
type MachineConfigInput struct { |
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.
don't need to cap MachineConfigInput
. it can be just machineConfigInput
.
starting with cap means it is "public"
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.
done
reefd/main.go
Outdated
Configuration string `json:"configuration"` | ||
} | ||
|
||
type MachineConfig struct { |
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.
same here: machineConfig
.
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.
done
@@ -0,0 +1,124 @@ | |||
package main |
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.
you would like to apply go fmt
to the file
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.
done
reefd/main.go
Outdated
rows, err = db.Query("SELECT id, name, configuration FROM machine_config") | ||
} | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusInternalServerError) |
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.
you do not want to return internal sql errors directly to user.
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.
done. I send the sql errors to log instead and only mention internal error
reefd/main.go
Outdated
defer rows.Close() | ||
|
||
// Iterate through the queried rows and print out | ||
var machineConfigs []MachineConfig |
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.
use []*MachineConfig
. this avoids copy.
in general, use struct pointers for most cases.
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.
done
reefd/main.go
Outdated
http.Error(w, err.Error(), http.StatusInternalServerError) | ||
return | ||
} | ||
for _, machineConfig := range machineConfigs { |
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.
use single char var name for machineConfig
, like c
.
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.
done
reefd/main.go
Outdated
err := json.NewDecoder(r.Body).Decode(config) | ||
if err != nil { |
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.
if err := xxxx ; err != nil {
}
this limits the scope of err
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.
done
reefd/main.go
Outdated
func main() { | ||
dbPath := flag.String("db", "reef.db", "Path to .db file") | ||
flag.Parse() | ||
if _, err := os.Stat(*dbPath); os.IsNotExist(err) { |
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.
you want to handle other types of error too; don't drop error
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.
if _, err := os.Stat(*dbPath); err != nil {
if os.IsNotExist(err) {
log.Fatal("not exist")
} else {
log.Fatalf("stat %q db file: %s", *dbPath, err)
}
}
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.
done
reefd/main.go
Outdated
configuration string | ||
} | ||
|
||
// a function to connect to database given a path to .db file |
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.
// connectToDatabase connects to database ...
reefd/main.go
Outdated
} | ||
}) | ||
log.Printf("Starting server") | ||
http.ListenAndServe(":1234", nil) |
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.
handle the err being returned.
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.
done
Signed-off-by: kevin <[email protected]>
} | ||
}) | ||
log.Printf("Starting server") | ||
if err := http.ListenAndServe(":1234", nil); err != nil { |
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.
make listen address a flag please?
and maybe use 8000 or 8080 or 80xx as the default port.
} | ||
|
||
// handleGetRequest processes GET request to /machine/configuration that query the machine config based on name | ||
func handleGetRequest(db *sql.DB, w http.ResponseWriter, r *http.Request) { |
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.
could you define a server struct/class, with db wrapped in it? and make this a method of that server?
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.
could you add some tests and fix the unit test?
GET /machine/configuration
andPOST /machine/configuration
curl -X POST -H "Content-Type: application/json" -d '{"name":"machine3","configuration":"config3"}' http://localhost:1234/machine/configuration
curl -X GET http://localhost:1234/machine/configuration
curl -X GET http://localhost:1234/machine/configuration?name=machine3