Remove dependency on go-baseapp #152
Conversation
godel/config/dist-plugin.yml
Outdated
| - os: linux | ||
| arch: amd64 | ||
| - os: darwin | ||
| arch: amd64 |
There was a problem hiding this comment.
I'm not sure why we ever had the build/disters for this library.
godel/config/test-plugin.yml
Outdated
| @@ -2,3 +2,6 @@ tags: | |||
| integration: | |||
| names: | |||
| - ^integration$ | |||
| exclude: | |||
| names: | |||
| - ^example$ | |||
There was a problem hiding this comment.
Without this,
$ ./godelw test
Error: [go list ./appconfig ./example ./githubapp ./oauth2] failed: main module (github.com/palantir/go-githubapp) does not contain package github.com/palantir/go-githubapp/example
: exit status 1
Should we just skip tests on example? Or is there a more correct way to enable tests and not have godel explode?
|
Yuck, is there a nice/correct way to tell godel to ignore the example module or treat it separately other than just a bunch of excludes in the check plugin? I've added 7b05511, but I don't like it. |
|
You can exclude directories from all processing using the No idea if this will work, but I'd try defining multiple projects in Godel, which I think is also the original reason we had the Another option is to leave |
asvoboda
changed the title
There was a problem hiding this comment.
Removing baseapp was a smaller change than I thought it would be! A few minor suggestions, but otherwise this looks good.
Also, do you think it's worth adding a sentence to the README plugging go-baseapp now that we don't reference it in code? Maybe right before the "Examples" header at the end of the "Usage" section.
| @@ -72,7 +72,6 @@ commenting with a copy of the comment body. | |||
| To run the app, update `example/config.yml` with appropriate secrets and then | |||
| run: | |||
|
|
|||
| ./godelw dep | |||
There was a problem hiding this comment.
I guess it's been a while since we updated this... 🙈
example/config.yml
Outdated
| @@ -6,7 +6,7 @@ github: | |||
| v3_api_url: "https://api.github.com/" | |||
| app: | |||
| integration_id: 0 | |||
| webhook_secret: "your-app-webhook-secret-here" | |||
|
|
||
| cc, err := githubapp.NewDefaultCachingClientCreator( | ||
| config.Github, | ||
| githubapp.WithClientUserAgent("example-app/1.0.0"), | ||
| githubapp.WithClientTimeout(3*time.Second), | ||
| githubapp.WithClientCaching(false, func() httpcache.Cache { return httpcache.NewMemoryCache() }), | ||
| githubapp.WithClientMiddleware( | ||
| githubapp.ClientMetrics(server.Registry()), |
There was a problem hiding this comment.
Might be nice to create a metrics.Registry to pass in here as a demonstration, even if there's nothing exporting the metrics.
example/main.go
Outdated
| logger.Info().Msg("Starting server...") | ||
| err = http.ListenAndServe(fmt.Sprintf("%s:%d", config.Server.Address, config.Server.Port), nil) |
There was a problem hiding this comment.
| logger.Info().Msg("Starting server...") | |
| err = http.ListenAndServe(fmt.Sprintf("%s:%d", config.Server.Address, config.Server.Port), nil) | |
| addr := fmt.Sprintf("%s:%d", config.Server.Address, config.Server.Port) | |
| logger.Info().Msgf("Starting server on %s...", addr) | |
| err = http.ListenAndServe(addr, nil) |
That way people don't have to refer to the configuration to know what port to use.
| if err != nil { | ||
| panic(err) | ||
| } | ||
| zerolog.DefaultContextLogger = &logger |
There was a problem hiding this comment.
Nice, this is a lot simpler than adding middleware only to put the logger in the context
Resolves #121