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
Move pg operations(commands) from ssh to http #64
Conversation
pkg/commands/handler.go
Outdated
http.HandleFunc("/commands/users/create", handleCreateUser) | ||
http.HandleFunc("/commands/users/delete", handleDeleteUser) | ||
|
||
http.HandleFunc("/commands/users/grant", handleGrantAccess) |
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 can ditch the grant/revoke handlers, these operations should be handled by user create/delete.
* Using go-chi for routing * Revert changes done to fly.toml * Serving from 5500 only
r.HandleFunc("/flycheck/vm", runVMChecks) | ||
r.HandleFunc("/flycheck/pg", runPGChecks) | ||
r.HandleFunc("/flycheck/role", runRoleCheck) | ||
|
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.
What are your thoughts on combining the command/check handlers into a single file and keeping them under the same port?
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.
@davissp14 I've put them on the same port but they are still in different packages that are imported by a new package pkg/server
. I could rename it though.
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.
Found a little bug with the attach process.
|
||
name := chi.URLParam(r, "name") | ||
|
||
db, err := admin.FindDatabase(r.Context(), pg, name) |
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.
This seems to return status code 500 when the the target database doesn't exist.
Flyctl expects 404: https://github.com/superfly/flyctl/blob/be327db0075b8e4bb35eda4c8359da0936b0c42d/pkg/flypg/pg.go#L92
I've tested out list users and list databases and both look good! I haven't been able to fully test attach/detach due to an issue noted in the last review.
|
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.
Nice work!
superfly/flyctl#893