[cmd/opampsupervisor] Refactor NewSupervisor function #34597
Conversation
bsponge
requested review from
evan-bradley,
atoulme and
tigrannajaryan
as code owners
There was a problem hiding this comment.
This is working great, thanks for picking this up.
I think Evan is out for the time being, @tigrannajaryan @atoulme are either of you able review?
crobert-1
added
the
ready to merge
mx-psi
removed
the
ready to merge
| } | ||
|
|
||
| if err := s.config.Validate(); err != nil { | ||
| if err := cfg.Validate(); err != nil { |
There was a problem hiding this comment.
would it make sense to also move the validation into the new Load function introduced in this PR?
This would make it easier to further decouple the supervisor from other components that also use the config, such as the Commander - if we already know the config is valid before calling NewSupervisor, we can use the config to create a Commander and inject that into the Supervisor via the NewSupervisor function.
There was a problem hiding this comment.
I added validation to Load. I wasn't sure if I should remove it from NewSupervisor but I think as long as it's possible to create config by hand (not using Load) it's better to have this.
There was a problem hiding this comment.
Thanks @bsponge. I have one request, but it's not a blocker.
| err = os.WriteFile(executablePath, []byte{}, 0o600) | ||
| require.NoError(t, err) | ||
|
|
||
| configuration := ` |
There was a problem hiding this comment.
Could you use one of the templated config files in the testdata directory for this config?
|
Looks like this got a bunch of approvals but never got merged. @bsponge if you get some time, could you bring this PR up to date? Would love to get this in. Looks like the CI failure is saying to run |
|
Thanks @bsponge! |
…#34597) **Description:** Pass config structure to `NewSupervisor` instead of a file path. **Link to tracking Issue:** open-telemetry#34379 **Testing:** I run tests and adjusted the existing ones **Documentation:** - --------- Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Description: Pass config structure to
NewSupervisorinstead of a file path.Link to tracking Issue: #34379
Testing: I run tests and adjusted the existing ones
Documentation: -