go-fed / apcore

Golang ActivityPub Server Framework
GNU Affero General Public License v3.0
104 stars 10 forks source link

Code review #26

Closed BenLubar closed 4 years ago

BenLubar commented 5 years ago

https://github.com/go-fed/apcore/blob/d66d825ea1a38f2f6a371f6d72a82b56515ca2ad/go.mod#L6

This version of go-fed/activity doesn't have the required github.com/go-fed/activity/streams/vocab package.

To update to the latest master, run go get github.com/go-fed/activity@master in the apcore directory.

https://github.com/go-fed/apcore/blob/d66d825ea1a38f2f6a371f6d72a82b56515ca2ad/database.go#L303-L305

This string is backwards. Trivial fix.

(side note: it would also be nice to be able to remember the password/lack of password in the config, even if it had to be added manually)

https://github.com/go-fed/apcore/blob/d66d825ea1a38f2f6a371f6d72a82b56515ca2ad/config.go#L148

This always crashes because c is nil and the database config can't be guessed by the ini package.

Proposed fix:

diff --git a/config.go b/config.go
index 3d9cc38..14e11d7 100644
--- a/config.go
+++ b/config.go
@@ -145,6 +145,10 @@ func loadConfigFile(filename string, a Application, debug bool) (c *config, err
        if err != nil {
                return
        }
+       c, err = defaultConfig(cfg.Section("database").Key("db_database_kind").String())
+       if err != nil {
+               return
+       }
        err = cfg.MapTo(c)
        if err != nil {
                return

it seems the current executable code path doesn't get any further than this, so I'll stop the review here for now

cjslep commented 5 years ago

Thanks for taking the time to review!

I'll take a look at this soon.

cjslep commented 4 years ago
  1. go.mod and go.sum updated in 3b582c740de672c7235a96de2b706f874bbd7083
  2. Fixed user and DB name in f18f826073eb604475972097a959d5164e54ae9b
  3. Initialized the config object in f7093b7d38976aad9cc54593b94f4c12d8ce178e
cjslep commented 4 years ago

There isn't any new functionality hooked up at this time, but I'm thinking of spending a few hours tonight doing so. I'll close this issue, but please do more code reviews (either open this one again, or create a new issue).