-
Notifications
You must be signed in to change notification settings - Fork 136
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: Config to Application State #138
Refactor: Config to Application State #138
Conversation
I'm still digging into details as there's a lot here, but just want to say this is some damn nice work @justenwalker |
What I've noticed is that there are broadly two categories of methods there. There are some that are thin wrappers on The second category are those that are building |
@@ -58,90 +40,19 @@ type Config struct { | |||
BackendsConfig json.RawMessage `json:"backends,omitempty"` | |||
TasksConfig json.RawMessage `json:"tasks,omitempty"` | |||
TelemetryConfig json.RawMessage `json:"telemetry,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily in scope for this PR, but omitempty
is really only used when we serialize a struct, right? Maybe we don't really need this annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can remove it - we really don't re-serialize the config anyway, so the extra tag options are mostly useless.
omitempty is used for deserialization, but we never deserialize the config once it is loaded, so there is no need to tag these structures with this tag.
@tgross There is a lot here. I broke it out into individual commits that should be hopefully self-explanitory, but I'll summarize:
|
"os/exec" | ||
|
||
"github.com/joyent/containerpilot/utils" | ||
"github.com/prometheus/common/log" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prometheus/common/log? What happened to logrus here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my auto-importer often gets the common names wrong :(
One last little comment about the log import in |
LGTM. |
The native golang HTTP server cannot be gracefully reloaded. The telemetry server originally punted on reloading because this meant the only thing we couldn't change was the telemetry port. Although we made a go at it, we can't make the reload work so this commit backs out the change (but still uses the refactored code from PR TritonDataCenter#138) until we can find a better HTTP server lib to use.
For #128
This is an initial stab at factoring out the application state from configuration. One cool thing about this is that many (all?) of the global variables go away.
I also added support for telemetry reload (hopefully, I have not tested this extensively)
I thought I'd get this out in the open so that everyone can poke some holes in it.
cc: @tgross
Work in progress
One thing I'm not too happy about is the proliferation of methods that I implemented on the
Config
struct. I think the next step would probably be to push that logic into the packages themselves. I've had some success implementing configs as some high-level structure containing mostly maps and using github.com/mitchellh/mapstructure - since it supports weak typing which would be good for things like #122