Issue-189 : Allow connecting to bookmarked server from CLI #201

akarki15 · 2016-11-11T01:09:07Z

Resolves #189

Tested that the feature works -

▶ cat ~/.pgweb/bookmarks/server.toml
host = "localhost"
port = 5432
user = "postgres"
database = "nice2"
ssl = "disable"

▶ ./pgweb --bookmark=server
Pgweb v0.9.5
Connecting to server...
Checking database objects...
Starting server...
To view database open http://localhost:8081/ in browser
[GIN] 2016/11/10 - 01:11:30 | 200 |  17.896837ms | 127.0.0.1:63872 |   GET     /
[GIN] 2016/11/10 - 01:11:30 | 200 |   1.374726ms | 127.0.0.1:63872 |   GET     /static/css/bootstrap.css
[GIN] 2016/11/10 - 01:11:30 | 200 |    603.253µs | 127.0.0.1:63878 |   GET     /static/css/font-awesome.css
[GIN] 2016/11/10 - 01:11:30 | 200 |    584.902µs | 127.0.0.1:63879 |   GET     /static/css/app.css
[GIN] 2016/11/10 - 01:11:30 | 200 |     894.82µs | 127.0.0.1:63880 |   GET     /static/js/jquery.js
[GIN] 2016/11/10 - 01:11:30 | 200 |   1.833552ms | 127.0.0.1:63881 |   GET     /static/js/ace.js

We will use this method in upcoming commit to create client from stored bookmark.

Given a bookmark path and bookmark name, GetBookmark returns a Bookmark object that corresponds to the stored bookmark settings. In next commits, we will use this method to read stored bookmarks and create a db client.

We know that a port is a number. Lets enforce that rule at type level by setting it so. This commit also adjusts test funcs and helper data to fit Port's new int type.

We will use client.NewFromURL to create a client from stored bookmark in next commit. Since client.NewFromURL takes in a command.Options, lets add a method on Bookmark to get corresponding Option.

if options.Bookmark is set, initClient will now use it to create a client from the bookmarked settings. initClientUsingBookmark uses methods introduced in previous commits. It constructs connection string to db from Bookmark.Url if it exists. If not, it uses other fields in Bookmark to construct the connection string. An implication of this is that the Url field of Bookmark takes precedence.

sosedoff · 2016-11-11T02:35:03Z

@akarki15 I see that you've changed port handling in the bookmarks. Its a breaking change since any existing bookmarks will error out with Type mismatch for 'bookmarks.Bookmark.Port': Expected integer but found 'string'..

sosedoff · 2016-11-11T02:37:39Z

Also, what happens if i specify invalid bookmark (typo, name mismatch, forgot extension, etc). Right now i get Error: dial tcp [::1]:0: connect: can't assign requested address or a panic

akarki15 · 2016-11-11T03:35:23Z

@sosedoff that's because the port no. in the toml file should be a number. see the example in data folder

akarki15 · 2016-11-11T03:40:41Z

if the port no. is of type string, the user can still pass a non integer. its just that the error would get caught later when we'd be doing strings.Atoi().

What bookmark settings is giving you Error: dial tcp [::1]:0: connect: can't assign requested address?

sosedoff · 2016-11-11T16:04:32Z

Regarding the change to int - change itself is fine (although its breaking right now) but in future you should keep the PRs scoped to a single feature.

As for the error: i removed all bookmarks from the path and specified -b dummy pgweb flag. Results in panic.

sosedoff · 2016-11-11T16:05:33Z

pkg/bookmarks/bookmarks.go

@@ -72,3 +89,22 @@ func ReadAll(path string) (map[string]Bookmark, error) {

 	return results, nil
 }
+
+type ErrNonExistingBookmark string


Do not use this error definition pattern. Its not being used anywhere in the codebase.

i see your point about not using a new pattern to keep things consistent. i would however like to point the advantage of this pattern - having an explicit typed error means that in the unit tests, i can assert equality on types of error rather than doing a vague string assertion. example here -

assert.Equal(t, ErrNonExistingBookmark("bar"), err)

68db934 gets rid of this err pattern

sosedoff · 2016-11-11T16:07:28Z

pkg/bookmarks/bookmarks.go

+	}
+	bookmark, ok := bookmarks[bookmarkName]
+	if !ok {
+		return Bookmark{}, ErrNonExistingBookmark(bookmarkName)


When bookmark does not exist, this line triggers panic. It should really be

return Bookmark{}, fmt.Errorf("bookmark does not exist:", bookmarkName)

i found the reason for panic. i should have type casted error to string before passing it to sprintf-

func (e ErrNonExistingBookmark) Error() string { return fmt.Sprintf("couldn't find a bookmark with name %s", string(e)) }

68db934 - doesn't panic anymore if you pass non existing file

github.com/sosedoff/pgweb Issue-189-take-2 ✔ 3m ▶ ./pgweb --bookmark=dummy Pgweb v0.9.5 Error: couldn't find a bookmark with name dummy

The codebase doesn't yet use the explicitly typed error pattern. To keep things consistent, lets use the generic error type.

sosedoff · 2016-11-15T02:34:51Z

pkg/bookmarks/bookmarks.go

+	return b.Ssh.User == "" && b.Ssh.Host == "" && b.Ssh.Port == ""
+}
+
+func (b Bookmark) ConvertToOptions() (command.Options, error) {


Whats the point in returning error here ?

you're right. no point in returning error here. forgot to remove the error when i changed port from string to int

ConvertToOptions can't return any error.

akarki15 added 5 commits November 10, 2016 20:06

Add method that returns if bookmark's ssh is empty

f4fb574

We will use this method in upcoming commit to create client from stored bookmark.

Add a func to read bookmark from stored toml file

0e88e3e

Given a bookmark path and bookmark name, GetBookmark returns a Bookmark object that corresponds to the stored bookmark settings. In next commits, we will use this method to read stored bookmarks and create a db client.

Set the type of Bookmark.Port to int

038cb62

We know that a port is a number. Lets enforce that rule at type level by setting it so. This commit also adjusts test funcs and helper data to fit Port's new int type.

Add a method to convert bookmark to options

61dfe67

We will use client.NewFromURL to create a client from stored bookmark in next commit. Since client.NewFromURL takes in a command.Options, lets add a method on Bookmark to get corresponding Option.

sosedoff reviewed Nov 11, 2016

View reviewed changes

akarki15 added 2 commits November 12, 2016 12:30

Use generic typed error pattern

68db934

The codebase doesn't yet use the explicitly typed error pattern. To keep things consistent, lets use the generic error type.

Make assets.

b1fdbf7

sosedoff reviewed Nov 15, 2016

View reviewed changes

Remove error return value from ConvertToOptions

fedde80

ConvertToOptions can't return any error.

sosedoff merged commit d74bc06 into sosedoff:master Nov 16, 2016

akarki15 deleted the Issue-189-take-2 branch May 11, 2020 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue-189 : Allow connecting to bookmarked server from CLI #201

Issue-189 : Allow connecting to bookmarked server from CLI #201

akarki15 commented Nov 11, 2016

sosedoff commented Nov 11, 2016

sosedoff commented Nov 11, 2016

akarki15 commented Nov 11, 2016

akarki15 commented Nov 11, 2016

sosedoff commented Nov 11, 2016

sosedoff Nov 11, 2016

akarki15 Nov 12, 2016

akarki15 Nov 12, 2016

sosedoff Nov 11, 2016

akarki15 Nov 12, 2016

akarki15 Nov 12, 2016 •

edited

akarki15 Nov 12, 2016

sosedoff Nov 15, 2016

akarki15 Nov 15, 2016

akarki15 Nov 16, 2016

Issue-189 : Allow connecting to bookmarked server from CLI #201

Issue-189 : Allow connecting to bookmarked server from CLI #201

Conversation

akarki15 commented Nov 11, 2016

sosedoff commented Nov 11, 2016

sosedoff commented Nov 11, 2016

akarki15 commented Nov 11, 2016

akarki15 commented Nov 11, 2016

sosedoff commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akarki15 Nov 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akarki15 Nov 12, 2016 •

edited