Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor handlers: make Config not global #67

Merged
merged 2 commits into from
Jun 11, 2018
Merged

Refactor handlers: make Config not global #67

merged 2 commits into from
Jun 11, 2018

Conversation

mholt
Copy link
Contributor

@mholt mholt commented Apr 13, 2018

I know this diff looks huge, but it's actually pretty simple: instead of a global value named Config, the identifier named Config is now a struct type. Instead of having all sorts of global configuration, we confine it to that struct... and attach the handlers as methods on the struct.

It's a lot cleaner, in that CLI options are confined to main and the main() function just builds a Config struct like any other user of the library would.

This makes it possible to use the restserver package as a library.

The default configuration is still preserved.

Closes #66

/cc @rawtaz

@fd0
Copy link
Member

fd0 commented Apr 15, 2018

This already looks very good, thanks! I've got a few comments:

  • In main.go, we can just have a global instance of Config (e.g. called cfg), which can then be filled via the Cobra flags. This way, we don't have to define all the variables again. Would you mind changing that? Example in restic: https://github.com/restic/restic/blob/a9c2e84ccd664a44fa268e209c2f2a44d40d020b/cmd/restic/global.go#L67

  • One thing that bugs me: Now we have a Config type which implements an HTTP server, that feels wrong to me. How about a Server with a Config field, so the methods are attached to the Server?

@mholt
Copy link
Contributor Author

mholt commented Apr 15, 2018

@fd0

In main.go, we can just have a global instance of Config (e.g. called cfg), which can then be filled via the Cobra flags. This way, we don't have to define all the variables again.

Great idea -- I'll get on that!

One thing that bugs me: Now we have a Config type which implements an HTTP server, that feels wrong to me. How about a Server with a Config field, so the methods are attached to the Server?

I can do that. It probably "feels wrong" because it's named Config, but it might as well be named Server or Handler. :) What if we just rename it?

@mholt
Copy link
Contributor Author

mholt commented Apr 15, 2018

I went ahead and renamed Config to Server, and used a single instance of it in main -- I do like this better. :)

@mholt
Copy link
Contributor Author

mholt commented May 7, 2018

I know you've been busy @fd0, so just a friendly ping in case this got lost in all the shuffle. :)

@mholt
Copy link
Contributor Author

mholt commented May 31, 2018

Any other changes you need for this? I've been using this for some time and it is working well for me.

Copy link
Member

@fd0 fd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so much better, thank you! It indeed bugged me because it was named Config. Server is a great choice.

@fd0 fd0 merged commit df3b6aa into restic:master Jun 11, 2018
fd0 added a commit that referenced this pull request Jun 11, 2018
Refactor handlers: make Config not global
@mholt
Copy link
Contributor Author

mholt commented Jun 11, 2018

Yay!! Thanks!

claui added a commit to claui/caddy that referenced this pull request Jul 16, 2018
In June 2018, dependency rest-server[1] cleaned up its API a
little[2], breaking the Caddy restic plugin build in the process.

Attempting to build Caddy with the plugin leads to the following
errors:

```
build/src/github.com/restic/caddy/setup.go:24:4: undefined: restserver.Config
build/src/github.com/restic/caddy/setup.go:40:19: undefined: restserver.NewMux
```

This commit applies the upstream API changes to fix the errors.

[1]: https://github.com/restic/rest-server
[2]: restic/rest-server#67
claui added a commit to claui/caddy that referenced this pull request Jul 16, 2018
In June 2018, dependency rest-server[1] cleaned up its API a
little[2], breaking the Caddy restic plugin build in the process.

Attempting to build Caddy with the plugin leads to the following
errors:

```
build/src/github.com/restic/caddy/setup.go:24:4: undefined: restserver.Config
build/src/github.com/restic/caddy/setup.go:40:19: undefined: restserver.NewMux
```

This commit applies the upstream API changes to fix the errors.

[1]: https://github.com/restic/rest-server
[2]: restic/rest-server#67
mholt pushed a commit to restic/caddy that referenced this pull request Aug 1, 2018
In June 2018, dependency rest-server[1] cleaned up its API a
little[2], breaking the Caddy restic plugin build in the process.

Attempting to build Caddy with the plugin leads to the following
errors:

```
build/src/github.com/restic/caddy/setup.go:24:4: undefined: restserver.Config
build/src/github.com/restic/caddy/setup.go:40:19: undefined: restserver.NewMux
```

This commit applies the upstream API changes to fix the errors.

[1]: https://github.com/restic/rest-server
[2]: restic/rest-server#67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants