Base implementation of the structure of Eiffel-Goer.

[t-persson]

Applicable Issues

N/A

Description of the Change

Only one handler and endpoint implemented currently, this is
the v1alpha1/events/id/ endpoint.
The other endpoints shall be implemented in future PRs
Only added support for MongoDB, more databases can be
implemented easily in the future.
No tests for the MongoDB implementation. I will add them
after this PR.

Alternate Designs

Benefits

An open source event repository API implementation.

Possible Drawbacks

None!

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

Signed-off-by: Tobias Persson [email protected]

---

This change is 

[magnusbaeck]

Looks good overall. I'm commenting on lots of things here but not everything needs to be addressed in this PR.

Since we're actually building this against an existing OpenAPI spec I wonder if it would be beneficial to generate code with go-swagger.

[magnusbaeck]

Use git describe --always instead? Then it'll pick up tags if they exist.

Also, strongly suggest use of govvv to version-stamp the binary itself (but that can be added later).

[t-persson]

Changing to git describe --always.
How are we going to use govvv? Just use it to do "go build" or do we want to use the information somehow? Maybe as extra information to logrus ?

[magnusbaeck]

I see two reasons for version-stamping the binary:

• Creating a logging field with the version to make it easy to see exactly which binary emitted a particular message and e.g. be able to compare relative frequencies of messages between versions (for canary evalution etc).
• Being able to unambiguously know exactly which binary is running. I've certainly run into cases where there have been weird and unexpected errors and when it's been soothing to be able to confirm what code is actually running.

Again, we can do this in a follow-up.

[t-persson]

Done

[magnusbaeck]

Why do we need this? I'd expect the ID field to get populated by the preceding Decode call.

[t-persson]

It's needed because it's not a Query parameter, but a "path" parameter.
I.e. instead of http://goer/events?id=uuid it's http://goer/events/uuid as per the swagger specification of ER /events/{id}

[magnusbaeck]

Ah, of course. And the ID field in EventsSingleRequest isn't expected to be populated in Decode but exists so that we can populate it from vars["id"] and keep all request parameters in a single struct? If so, perhaps we should drop the schema tag for the ID struct member to make it clear that we don't want it to be populated by Decode?

[t-persson]

Done

[t-persson]

Looks good overall. I'm commenting on lots of things here but not everything needs to be addressed in this PR.

Since we're actually building this against an existing OpenAPI spec I wonder if it would be beneficial to generate code with go-swagger.

Sadly go-swagger does not support OpenAPI 3 and does not seem to want to support it in the future either.

[magnusbaeck]

I recommend using github.com/stretchr/testify for assertions instead of the standard stuff in testing.T. That way this rather cumbersome conditional can be turned into this:

assert.Equal(t, testCase.statusCode, responseRecorder.Code)

(Doesn't have to be addressed in this PR.)

[t-persson]

I will write an issue on this after we've merged the PR

[magnusbaeck]

Ah, of course. And the ID field in EventsSingleRequest isn't expected to be populated in Decode but exists so that we can populate it from vars["id"] and keep all request parameters in a single struct? If so, perhaps we should drop the schema tag for the ID struct member to make it clear that we don't want it to be populated by Decode?

[magnusbaeck]

I had forgotten that we want to be open to supporting additional database types. If we want to keep database.go scheme-agnostic we could define an interface like

type DatabaseDriver interface {
        func SupportsScheme(scheme string) bool
        func Get(connectionURL *url.URL, logger *log.Entry) (Database, error)
}

supply an implementation of this in each database driver, and reference each such implementation in database.go:

var drivers = []DatabaseDriver{mongodb.Driver{}}
...
for _, driver := range drivers {
        ...
        if driver.SupportsScheme(connectionURL.Scheme) {
                return driver.Get(connectionString, databaseName)
        }
}

Anyway, this is something we could do at a later stage if desired.

But one thing we should fix right away is mapping both "mongo" and "mongo+srv" to the mongodb driver, otherwise this won't work with our in-house deployment :-)

[magnusbaeck]

Submitting single comment since my review ended up in a weird state where I couldn't add additional comments.

[t-persson]

Reviewable status: 0 of 32 files reviewed, 23 unresolved discussions (waiting on @magnusbaeck)

---

Makefile, line 15 at r1 (raw file):

Previously, magnusbaeck (Magnus Bäck) wrote…

Oh, I see. "No test files" doesn't bother me but you can keep it this way.

Done.

---

Makefile, line 9 at r3 (raw file):

Previously, magnusbaeck (Magnus Bäck) wrote…

Nothing we have address now, but this'll pull the latest govvv every time we build which potentially could result in different results (or even errors) if an old commit it rebuilt later on (plus it adds latency). I think we could append @v0.3.0 to the package name in the go get command to always get that version. Another option is to track the version via go.mod and use a dummy file named e.g. tools.go that looks like

// +build tools
package tools
 
import (
        _ "github.com/ahmetb/govvv"
)

to make sure it isn't purged from go.mod, then have a makefile rule like

$(GOBIN)/govvv:
        go get github.com/ahmetb/govvv

to install the tool only once per workspace (and add $(GOBIN)/govvv as a prerequisite of the build rule). See also golang/go#25922.

We'll fix the Makefile in the future as Makefiles are still a bit of a mystery to me. I'll have to do research! :)

---

cmd/goer/main.go, line 40 at r3 (raw file):

Previously, magnusbaeck (Magnus Bäck) wrote…

I don't think we should rely on this environment variable to be available (see https://superuser.com/questions/132489/hostname-environment-variable-on-linux). Prefer os.Hostname() instead.

Done.

---

internal/database/database.go, line 57 at r1 (raw file):

Previously, magnusbaeck (Magnus Bäck) wrote…

I had forgotten that we want to be open to supporting additional database types. If we want to keep database.go scheme-agnostic we could define an interface like

type DatabaseDriver interface {
        func SupportsScheme(scheme string) bool
        func Get(connectionURL *url.URL, logger *log.Entry) (Database, error)
}

supply an implementation of this in each database driver, and reference each such implementation in database.go:

var drivers = []DatabaseDriver{mongodb.Driver{}}
...
for _, driver := range drivers {
        ...
        if driver.SupportsScheme(connectionURL.Scheme) {
                return driver.Get(connectionString, databaseName)
        }
}

Anyway, this is something we could do at a later stage if desired.

But one thing we should fix right away is mapping both "mongo" and "mongo+srv" to the mongodb driver, otherwise this won't work with our in-house deployment :-)

I made an update, but there were a few problems so we might want to revert that commit if you are not happy with it :)

We'd have to update the mongodb.Driver{}.Get(...) to return a Database which would be a circular import and not allowed, so I instead created a new drivers package to host DatabaseDriver and added the functions from Database to that new interface.

Of course this could be done in several other ways, and if we want to use another way of doing this then we should just revert my change and write an Issue. What do you think?

---

internal/database/drivers/mongodb/mongodb.go, line 58 at r1 (raw file):

Previously, magnusbaeck (Magnus Bäck) wrote…

Yes, that's how it's usually done. It takes a bit of time to get used to contexts but they're quite convenient.

Done.

---

internal/database/drivers/mongodb/mongodb.go, line 51 at r4 (raw file):

Previously, magnusbaeck (Magnus Bäck) wrote…

Dunno if there are any corner cases where this doesn't produce the desired result. I've used

cs, err := connstring.Parse(connString)
// error handling
cs.Database

elsewhere.

Done.

[magnusbaeck]

Just a couple of nits but nothing blocking.

We should probably consider moving some of the packages under pkg/ to internal/ unless we really want it to be fair game for others to import. For example, the Eiffel event definitions are temporary until eiffelevents-sdk-go is ready and then the file will be removed, so including it in a public package doesn't really make sense. Anyway, as long as we don't create a v1.0.0 tag we're free to move things around.

[magnusbaeck]

In the makefile we always push to a tag that matches git describe. Will we ever have a latest tag that would be matched by this command?

[t-persson]

I will leave it as is for now, but I see three scenarios that we could follow here:

1. Always push latest when we push a new tag.
2. Add an export VERSION=git describe command in the README.
3. Always update the README when doing new version tags.

The third option feels best, but I will forget to do it on releases :)

[t-persson]

Just a couple of nits but nothing blocking.

We should probably consider moving some of the packages under pkg/ to internal/ unless we really want it to be fair game for others to import. For example, the Eiffel event definitions are temporary until eiffelevents-sdk-go is ready and then the file will be removed, so including it in a public package doesn't really make sense. Anyway, as long as we don't create a v1.0.0 tag we're free to move things around.

Moved it.
I'll be merging this now and write issues for the rest.
I will also create a milestone for version v1.0.0 which will be to match the OpenAPI specification.

Thank you for your review!