Pass config by reference into New methods

[craigmiskell-gitlab]

Without this, the setting of 'CollectMetrics' after constructing the proxy does not work as expected (it is setting it on the original Config, not the copy that the proxy now has)

The underlyling bug could also be fixed by setting CollectMetrics before constructing the Proxy, but this seems like a reasonable way to make it work and be more robust as well (for the future)

Description

Pass the Config object around by reference, not by value, so that metrics get enabled as expected

Checklist

• [ ] Code compiles correctly
• [ ] Created tests (if appropriate)
• [ ] All tests passing
• [ ] Extended the README / documentation (if necessary)

[dropwhile]

hmm. Not passing by reference was intentional. The proxy instance is not expected to be be runtime tunable in that regard, and possibly dangerous if config became nil after the proxy was running. Things set in the internal http transport by New would also not be updated by changing config (even if it were pass by ref), as another problem. So having it be passed by reference would be confusing -- some things runtime changeable, some not.

If someone is consuming the code as a library, or modifying go-camo for their own purposes (eg. signal handler to enable/disable metrics), perhaps a better means that works with the currently code would be to construct a new proxy instance and replace the existing http endpoint target -- swapping out dumbrouter.CamoHandler for example.

If certain aspects are desired to be tunable, functions could be added to toggle them on/off, but there could be additional overhead involved if they need to be concurrency safe changes. Given that the cmd/go-camo instance is the predominant one, and the pkg code is not necessarily intended to be consumed as a library, and that go-camo itself doesn't expose runtime tune-ability, it seems like a bit of an unnecessary (and possibly dangerous) feature.

I suppose the lack of runtime tune-ability of Config should be documented though, just to avoid confusion.

---

If you do have a strong use-case for runtime tune-ability, I'm open to discussing which aspects would make sense to be tunable, but I don't think just making the config mutable after creation is a good one, given the current design.

[dropwhile]

The bug is indeed the order of configuration though, for sure.

diff --git a/cmd/go-camo/main.go b/cmd/go-camo/main.go
index aa390ad..3f0d4b7 100644
--- a/cmd/go-camo/main.go
+++ b/cmd/go-camo/main.go
@@ -277,17 +277,6 @@ func main() {
    config.MaxRedirects = opts.MaxRedirects
    config.ServerName = ServerName

-   proxy, err := camo.NewWithFilters(config, filters)
-   if err != nil {
-       mlog.Fatal("Error creating camo", err)
-   }
-
-   var router http.Handler = &router.DumbRouter{
-       ServerName:  ServerResponse,
-       AddHeaders:  AddHeaders,
-       CamoHandler: proxy,
-   }
-
    if opts.Metrics {
        config.CollectMetrics = true
        mlog.Printf("Enabling metrics at /metrics")
@@ -309,6 +298,17 @@ func main() {
        router = promhttp.InstrumentHandlerResponseSize(responseSize, router)
    }

+   proxy, err := camo.NewWithFilters(config, filters)
+   if err != nil {
+       mlog.Fatal("Error creating camo", err)
+   }
+
+   var router http.Handler = &router.DumbRouter{
+       ServerName:  ServerResponse,
+       AddHeaders:  AddHeaders,
+       CamoHandler: proxy,
+   }
+
    http.Handle("/", router)

    if opts.BindAddress != "" {

[craigmiskell-gitlab]

Re-ordering works for me; I don't have any use-case for run-time tunability, just looking for the smoothest way to make metrics work. I'll close this, and open a PR with the re-ordering (just to keep things clear in the commits, comments, and history)