appcontext/tracing: make InitContext public#4548
appcontext/tracing: make InitContext public#4548laurazard wants to merge 1 commit intomoby:masterfrom
InitContext public#4548Conversation
Make `detect.InitContext` public as opposed to only available through the use of contexts from `appcontext` so that downstream users (e.g. buildx) can keep the OTEL context utils without having to use `appcontext` - see: docker/buildx#2184 Signed-off-by: Laura Brehm <laurabrehm@hey.com>
| func init() { | ||
| appcontext.Register(initContext) | ||
| appcontext.Register(InitContext) | ||
| } |
There was a problem hiding this comment.
Wondering if we need to keep the init, as that would be dead-code if appcontext is not used (but it will remain a dependency due to it being imported) 🤔
(have not looked how / where it's imported though).
There was a problem hiding this comment.
Yeah, I thought about making other changes to appcontext/this package for that but had concerns about that possibly silently breaking things for other downstream users. If we do that, we should do that together with other changes to make sure we don't silently break, or clearly call it out.
There was a problem hiding this comment.
(side note, but I really hate inits for this reason, it's much harder to reason about and figure out the blast radius of changes like this)
jsternberg
left a comment
jsternberg
left a comment
There was a problem hiding this comment.
LGTM. My preference for the future is that we make an effort to refactor the tracing code so we're not seeing buildx or other docker utilities depend so heavily on buildkit for tracing utilities and improve the buildx code to have a less ambiguous way of configuring tracing, but this seems like a good way to handle the current problem without making the blast radius too large.
|
@jsternberg with this function exported, do you think it would make sense to move the (trying to see if we can get |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
let me LGTM because the other bits can still be worked on in a follow-up 🤞
|
@thaJeztah I don't know too much about it, but I assume it can probably be moved to another package. |
|
little ping, maybe we can get this one in and then open up another to move the init elsewhere so we can discuss that there? |
|
Per latest comments in docker/buildx#2184 , I don't think that PR needs this anymore. |
|
Closing based on discussion in docker/buildx#2184 |
5 participants
Make
detect.InitContextpublic as opposed to only available through the use of contexts fromappcontextso that downstream users (e.g. buildx) can keep the OTEL context utils without having to useappcontext- see: docker/buildx#2184