Buildkite::TestCollector.tag_execution(key, value)#240
Conversation
Buildkite::TestCollector.tag_execution(key, value)
ffc22e2 to
1176ad4
Compare
| Thread.current[:_buildkite_tags] = nil | ||
|
|
||
| if !tracer.nil? | ||
| Thread.current[:_buildkite_tracer] = nil |
There was a problem hiding this comment.
I wonder if there was a reason that this is put behind this if nil condition?
There was a problem hiding this comment.
I think it's just that there's no point clearing it if it was nil to begin with.
But AFAIK there's also no harm in it, and grouping them together in source code felt better.
Something worth noting: Thread.current[] works differently to other things in Ruby; there's no “delete” operation for a key; assigning nil actually deletes it, so this line changes Thread.current.key?(:_buildkite_tracer) from true to false.
| tags = Thread.current[:_buildkite_tags] | ||
| raise "_buildkite_tags not available" unless tags | ||
|
|
||
| unless key.is_a?(String) && value.is_a?(String) |
There was a problem hiding this comment.
Should we consider supporting String like classes like numbers and booleans?
Should we be more strict about sanitisation or should we handle that server side?
There was a problem hiding this comment.
Unsure, so I started strict and we can expand it to be more permissive without breaking anything for anyone.
(Can't do the other direction)
Draft support for per-execution tagging.
Buildkite::TestCollector.tag_execution(key, value)is an okay API for this.I don't love adding yet another
Thread.current[:_buildkite…]fiber-local (global-ish) name, but I don't see a really good place to put these tags otherwise, and really it's an inconsequential implementation detail, compared to the public API in front of it. I don't think they really belong inBuildkite::TestCollector::Tracer(we're tagging the execution, not the potentially-nested traces therein) and adding them there just adds complexity.