feat: add /env endpoint to allow exposing operator-controlled info from the server

[mloskot]

Proposal of implementation of the feature request

• feat: add /env endpoint to allow exposing operator-controlled info from the server #114

---

I've submitted this for an initial round of review.
I still have to add more cases to the test and add the endpoint to the index page, but first I'd like to receive feedback if this implementation goes in the right direction.

---

make run

curl http://127.0.0.1:8080/env
{
  "env": {}
}

export HTTPBIN_A=a
export HTTPBIN_B=b
export HTTPBIN_123=123

make run

curl http://127.0.0.1:8080/env
{
  "env": {
    "HTTPBIN_123": "123",
    "HTTPBIN_A": "a",
    "HTTPBIN_B": "b"
  }
}

[codecov]

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.07%. Comparing base (34a21a3) to head (9493ea4).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
httpbin/cmd/cmd.go	91.42%	2 Missing and 1 partial ⚠️
httpbin/options.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
- Coverage   95.20%   95.07%   -0.13%     
==========================================
  Files          10       10              
  Lines        1750     1766      +16     
==========================================
+ Hits         1666     1679      +13     
- Misses         48       51       +3     
  Partials       36       36              

Files with missing lines	Coverage Δ
httpbin/handlers.go	99.23% <100.00%> (+<0.01%)	⬆️
httpbin/httpbin.go	100.00% <100.00%> (ø)
httpbin/cmd/cmd.go	89.65% <91.42%> (+0.60%)	⬆️
httpbin/options.go	91.66% <0.00%> (-8.34%)	⬇️

[mccutchen]

Thanks for checking in on approach before investing too much work here!

While this implementation is nice and straightforward, I think I'd prefer a slightly different approach where the environment is resolved at process startup and passed explicitly to httpbin.New() via a new httpbin.WithEnv(env map[string]string) option func.

The general idea here is to do the work once instead of on every request when we can (sort of like how we pre-render all templates or only look up the hostname once rather than on every request).

Does that make sense to you?

[mloskot]

@mccutchen

I think I'd prefer a slightly different approach where the environment is resolved at process startup and passed explicitly to httpbin.New() via a new httpbin.WithEnv(env map[string]string) option func.

This is a very good suggestion indeed. I will update this PR with the process startup approach.

[mloskot]

@mccutchen I've refatored and the HTTPBIN_ vars are now collected at the startup. I'll welcome any feedback - I'm available to implement any suggestions you may have.

[mccutchen]

A few more comments/questions/suggestions, please let me know what you think!

[mccutchen]

Thanks for your persistence with this, it looks good to me overall, just a couple of hopefully final comments below.

It does look like we need a test for properly parsing the environment in mainImpl, I don't think I see any at a glance?

[mccutchen]

This change adds a lot of unnecessary noise to the diff, I'd rather we take a different approach here. Since we're already defining an env for these test cases, let's just use that env for both our getEnvVal and getEnviron stubs?

I.e. do something like this below:

- gotCode := mainImpl(tc.args, func(key string) string { return tc.optionsEnv[key] }, func() []string { return tc.env }, tc.getHostname, buf)
+ gotCode := mainImpl(tc.args, func(key string) string { return tc.env[key] }, func() []string { return environSlice(tc.env) }, tc.getHostname, buf)

where environSlice is a simple little helper to convert map[string]string into a []string slice of key=val pairs.

[mloskot]

It does look like we need a test for properly parsing the environment in mainImpl, I don't think I see any at a glance?

Yes, correct, I haven't managed to figure how to utilise the mainImpl for testing of the /env yet with some prefabricated HTTPBIN_ENV_* variables.
Perhaps, the HTTPBIN_ENV_* environ should become a member of the config, then the testing would become part of loadConfig test cases.
I'll welcome any suggestions.

This change adds a lot of unnecessary noise to the diff (...) where environSlice is a simple little helper (...)

I have reverted that env and optionsEnv splitting and implemented your suggestion.

[mccutchen]

Yes, correct, I haven't managed to figure how to utilise the mainImpl for testing of the /env yet with some prefabricated HTTPBIN_ENV_* variables.
Perhaps, the HTTPBIN_ENV_* environ should become a member of the config, then the testing would become part of loadConfig test cases.

Ahhhhh yeah, I think it might be worth it to move this new env configuration onto the config struct, just to make it testable. Would you mind making that change? After that, I think we're good to go here!

[mloskot]

Would you mind making that change?

Yes, of course, I will continue near the end of this week.

[mloskot]

@mccutchen As promised, I've just reworked this PR to store the env in the config and expose it via HTTPBin, and covered it with tests based on loadConfig test cases.

I don't mind to keep working on polishing this PR until you're happy to merge it, no rush with accepting it.

[mccutchen]

👍 this came out really nice, thanks so much for your patience and persistence!

[mloskot]

Pleasure is mine, thanks!