Golang migration #23

kaustavdm · 2017-09-17T19:27:53Z

In a stable enough state to merge. Moved old node branch to legacy-node branch

Note: Do not squash merge. Retain individual commits.

Note: Circle CI is disabled currently. It's not running any tests. I have overriden merge requirements for master till this PR is merged or #21 is fixed.

Fixes #17

Signed-off-by: Kaustav Das Modak <[email protected]>

It checks whether there is already an account for given email Signed-off-by: Kaustav Das Modak <[email protected]>

dolftax · 2017-09-18T05:20:50Z

config/config.go

+}
+
+// ParseConfig parses a configuration file and loads into a config
+func ParseConfig(confPath string) (Config, error) {


Unnecessary. Viper handles this for you.

I always try to cut down external dependencies to a minimum, hence went ahead and wrote a basic config parsing script. Viper does the job, but we are also operating in a controlled environment, where we don't need all that abstraction. The only thing I'd want here is a merging function which can override config in JSON with environment variables (which Viper does, but it also does a lot more). In a stack where ser-de is so straightforward, I don't see much reason to add a dependency.

One argument against this view is to let go of the grunt work to external libraries and focus on the application. My counterpoint is that this approach is the highway to dependency hell and eventual tech debt.

Also, the config package is not stabilized yet. I wrote a bare minimum implementation that works, made sure it can be incrementally added and then moved on to the more important parts of the application.

Explained you a use case couple days ago. When storing configuration in an shared system (etcd). Deploying the changes on all nodes is not the right way.

Technical debt, really? Heh. Unecessary. But upto you.

Agreed on the deployment scenario. Though, we don't know if that will be an etcd store or something else. Also, if configuration is indeed shared and hosted separately, we may want a dedicated reader, say, if all config is published to etcd, then all what rig needs to know at launch is the link to etcd and the rest it can fetch via a etcd client. It does not need viper to do that. At that point, etcd can also be used for other purposes where viper falls short. This is what I mean by reducing abstraction and lowering tech debt eventually.

I am not saying no to viper. It definitely looks good. I'm skeptic of introducing abstractions where they were not necessary.

(Check differences between "necessary" and "sufficient" clauses)

dolftax · 2017-09-18T05:21:04Z

config/config.go

+}
+
+// CreateConfig generates a default JSON config file and writes to the given path
+func CreateConfig(confPath string) (Config, error) {


Similar to above issue.

dolftax · 2017-09-18T05:22:12Z

config/config.go

+		serverConfig{
+			Port:        8081,
+			JWTSecret:   "replacemewithanicelongstringthatyouwillnotsharewithothers",
+			PostgresURL: "postgres://user:pass@host/db",


Why hardcode this? This has to be decoupled from code.

This is decoupled. The values there are dummy values meant to be changed. The whole purpose of this function is generate a dummy JSON config and then go ahead and edit the values. This approach is testable, does not need to maintain a sample file, gets compiled with the binary and hence can be kept updated per deployment.

Decoupling is not always about building a separate module. It is functional. A config module is decoupled. It can read config and can suggest initial config.

Ideally, configuration should be maintained outside the code in general parsable formats (JSON, YAML, ..)

Configuration should be maintained externally. Configuration structure should be internal. This generates dummy values, but with the right structure. Think npm init -y.

dolftax · 2017-09-18T05:22:55Z

glide.yaml

+- package: golang.org/x/crypto
+- package: github.com/go-pg/pg
+  version: ^6.4.24
+- package: github.com/jinzhu/inflection


Highway to dependency hell.

Which one on the list? Keep in mind that I talk of reducing dependency wherever possible, while not necessarily making it zero. Each item on this list has been thoroughly researched, compared, tested and then added to the list. jinzhu/inflection is explicitly on the list because glide kept failing to detect that it is a dependency of go-pg/pg. If I find it otherwise, I'm more than happy to remove it.

Difference between necessary and sufficient.

dolftax · 2017-09-18T05:24:17Z

models/edgecluster.go

+package models
+
+// EdgeCluster stores DNS and location information for edge clusters
+type EdgeCluster struct {


Edge cluster? I would avoid using the name cluster as it contradicts with the indenepdent application.

They are edge clusters. Not to be confused with independence. I see a pattern come up in your comments so let me address this clearly.

"Independence" is the lack of being dictated every action. "Decoupling" is the ability to have atomic responsibilities. "Distributed" is the ability to coordinate and execute responsibilities beyond physical boundaries. "Isolated" means lack of connectivity. "Ignorance" is the lack of knowledge. Independent does not imply decoupled. Distributed does not imply independent. Distributed may need to be decoupled. But, neither of these require ignorance.

In this case, rig needs to have knowledge of what edge clusters are deployed. Agents are deployed in nodes in edge clusters. 1 edge cluster can have one or more agent nodes. Customers of xplex interact with edge clusters and not with agent nodes. So, rig needs this information to correctly tell customers where to stream. This information can be manually added in the initial stages and can later be automated.

So, in this case, rig operates independent and distributed. Agents are distributed, but dependent on rig; they perform actions on the edge nodes behalf of rig. Agents operate under the assumption that there are many other agents, but they are isolated from each other. Rig sees the whole picture and coordinates as needed. Agents have basic instructions to figure out how to perform certain actions locally and only pass on the result of those actions to the rest of the system.

dolftax · 2017-09-18T05:27:37Z

models/user.go

+		return false
+	}
+	return true
+}


dolftax · 2017-09-18T05:31:15Z

rest/middlewares.go

+)
+
+// Private key type to prevent context key collisions
+type key int


Better name?

Thought of it, but then decided to keep it as simple name in private scope. This was also suggested in some go blogs. I like the simplicity. Though, one possibility is to rename it to ctxKey. Does that make sense?

dolftax · 2017-09-18T05:33:54Z

rest/user_handler.go

+			Email:    r.FormValue("email"),
+		}
+		if err := u.SetPassword(r.FormValue("password")); err != nil {
+			log.Printf("Error setting user password. Reason: %s\n", err)


Use println. Needn't do \n.

println doesn't allow a format specifier. I usually use some format in logs. \n is a common method of adding line break as required.

Seperate by commas? log.Println("Error doing this", err)

dolftax · 2017-09-18T05:35:32Z

token/token.go

+		jwt.StandardClaims{
+			Issuer:    fmt.Sprintf("%d", u.ID),
+			Subject:   u.Username,
+			Audience:  "rig.xplex.me",


Pick from env (or) config

Nope, nope, nope. This is functional style dependency injection. Testable and atomic. Whoever supplies the secret can choose to do so from env or config.

dolftax · 2017-09-18T05:35:52Z

token/token.go

+		jwt.StandardClaims{
+			Issuer:    senderid,
+			Subject:   email,
+			Audience:  "rig.xplex.me",


Pick from configuration. r?

Why do you speak unspoken sentences?

Nope. See comment above.

Signed-off-by: Kaustav Das Modak <[email protected]>

Kaustav Das Modak added 30 commits September 5, 2017 01:45

Start move to Golang.

d58caf8

Signed-off-by: Kaustav Das Modak <[email protected]>

Start adding db conn and models

cdcacf0

Signed-off-by: Kaustav Das Modak <[email protected]>

Add user model and set up basic PG connection

adcc8a3

Signed-off-by: Kaustav Das Modak <[email protected]>

Set fully qualified root package name and update deps

6c32105

Signed-off-by: Kaustav Das Modak <[email protected]>

Remove NewUser constructor. Don't need it

e1618ab

Signed-off-by: Kaustav Das Modak <[email protected]>

Mark user password field as not null

817a38c

Signed-off-by: Kaustav Das Modak <[email protected]>

Add model and postgres connections with relations

8fe2d2a

Signed-off-by: Kaustav Das Modak <[email protected]>

Use fully qualified package name

0c5d4d4

Signed-off-by: Kaustav Das Modak <[email protected]>

Add EdgeCluster and AgentNode models

60c4cd3

Signed-off-by: Kaustav Das Modak <[email protected]>

Add JWT generators for user actions

a70fea4

Signed-off-by: Kaustav Das Modak <[email protected]>

Remove dummy methods from main()

47ed036

Signed-off-by: Kaustav Das Modak <[email protected]>

Update to fix go-pg default value statement

e9d748a

Signed-off-by: Kaustav Das Modak <[email protected]>

ParsedToken function and rename methods

4758fc3

Signed-off-by: Kaustav Das Modak <[email protected]>

Move token/usertoken to token

3cf1cd2

Signed-off-by: Kaustav Das Modak <[email protected]>

Rename RigClaims to TokenClaims

1445ca1

Signed-off-by: Kaustav Das Modak <[email protected]>

Simple config parser

f759c1a

Signed-off-by: Kaustav Das Modak <[email protected]>

Reorder go packages for server

521ec36

Signed-off-by: Kaustav Das Modak <[email protected]>

Setup REST response structure

8e9db29

Signed-off-by: Kaustav Das Modak <[email protected]>

Remove agent codebase from rig

575be94

Signed-off-by: Kaustav Das Modak <[email protected]>

Remove spec from rig

4dd794d

Signed-off-by: Kaustav Das Modak <[email protected]>

Update build.sh to build server only

1847612

Signed-off-by: Kaustav Das Modak <[email protected]>

Make repository into a single binary producer

82572bf

Signed-off-by: Kaustav Das Modak <[email protected]>

Log.Fatal if http server can't be launched

6516e08

Signed-off-by: Kaustav Das Modak <[email protected]>

Add simple middleware chaining with basic user routes

a69e4bb

Signed-off-by: Kaustav Das Modak <[email protected]>

Add middleware for required fields

969d8cd

Signed-off-by: Kaustav Das Modak <[email protected]>

Print message of starting HTTP server

744066a

Signed-off-by: Kaustav Das Modak <[email protected]>

Rename datastore package to models

f9f191d

Signed-off-by: Kaustav Das Modak <[email protected]>

Create schema on server start

62c3dbf

Signed-off-by: Kaustav Das Modak <[email protected]>

Shorthand methods for error and success response

d39d9ec

Signed-off-by: Kaustav Das Modak <[email protected]>

Implement create user route

f0fc35e

Signed-off-by: Kaustav Das Modak <[email protected]>

kaustavdm requested a review from dolftax September 17, 2017 19:28

Kaustav Das Modak added 5 commits September 18, 2017 02:47

Add auth middleware for verifying JWT

62ac6b5

Signed-off-by: Kaustav Das Modak <[email protected]>

Fix iat and iss values for JWT

20e5a85

Signed-off-by: Kaustav Das Modak <[email protected]>

Add rest method for user password update

758cd4b

Signed-off-by: Kaustav Das Modak <[email protected]>

Implement user auth rest endpoint

deae9b9

Signed-off-by: Kaustav Das Modak <[email protected]>

Add rest endpoint to generate invites

c33b3a0

It checks whether there is already an account for given email Signed-off-by: Kaustav Das Modak <[email protected]>

dolftax reviewed Sep 18, 2017

View reviewed changes

Rename key to ctxKey

391a844

Signed-off-by: Kaustav Das Modak <[email protected]>

dolftax approved these changes Sep 18, 2017

View reviewed changes

dolftax merged commit b4fce63 into master Sep 18, 2017

dolftax deleted the golang branch September 18, 2017 11:37

Golang migration #23

Golang migration #23

Conversation

kaustavdm commented Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dolftax Sep 18, 2017 • edited Loading

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

dolftax Sep 18, 2017 • edited Loading

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

dolftax Sep 18, 2017 • edited Loading

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

dolftax Sep 18, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustavdm Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

kaustavdm commented Sep 17, 2017 •

edited

Loading

dolftax Sep 18, 2017 •

edited

Loading

dolftax Sep 18, 2017 •

edited

Loading

dolftax Sep 18, 2017 •

edited

Loading

dolftax Sep 18, 2017 •

edited

Loading

kaustavdm Sep 18, 2017 •

edited

Loading