Add null-in-targets telemetry#5838
Open
denik wants to merge 6 commits into
Open
Conversation
Record whether the bundle config has any null value under the targets section, collected before target overrides are merged. Co-authored-by: Isaac
Move the null-under-targets check from the CollectNullTelemetry mutator into LoadFromBytes, so it runs on the raw (pre-normalization) YAML and captures scalar-typed nulls (like workspace.host:) that normalization would otherwise drop. The mutator now just surfaces the flag. The Root.Merge method carries the flag forward when included files are merged, so a null in any included file is also counted. Add acceptance tests in acceptance/bundle/null/ that capture the current merge behavior via bundle validate -o json; these will change when the null-removes-value semantics are introduced in a follow-up PR. Co-authored-by: Isaac
Contributor
Approval status: pending
|
Add a dev-job-field target that nullifies resources.jobs.my_job.description, alongside dev-run-as which nullifies the run_as struct, to cover both struct-level and scalar field nulls under resources. Add bundle deploy + print_telemetry_bool_values to each test so the null-in-targets signal is asserted end-to-end. Add a dev-empty case to scalar-field that uses an explicit empty string rather than YAML null, documenting that both currently leave the base value in place (via different code paths). Co-authored-by: Isaac
Collaborator
Integration test reportCommit: 42842b4
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
Replace the single null-in-targets bool with five granular signals: - null-in-targets.scalar: null on a scalar field (string/int/bool); normalization drops it and emits a warning - null-in-targets.complex: null on a struct-typed field at the top level of a target override (e.g. run_as:) - null-in-targets.map-key: null as a value in a user-keyed map field (e.g. variables.foo:, or a field inside a resource entry) - null-in-targets.array-index: null at a sequence index - null-in-targets.resource: null on an entire resource entry (targets.<T>.resources.<type>.<name>) The top-level null-in-targets flag is retained as the OR of all categories. The classification is based on path structure and normalization survival: scalars are dropped by normalization, all others survive. A two-pass walk (raw tree + normalized tree) distinguishes scalar from complex nulls. Add MapKey and Resource acceptance test cases to acceptance/bundle/null/map-key. Co-authored-by: Isaac
Replace dots with dashes in sub-key names to avoid ambiguity with the dyn path separator. Also shorten map-key → key and array-index → index. Co-authored-by: Isaac
Co-authored-by: Isaac
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Record whether the bundle config has any null value under the
targetssection, surfaced as a bool telemetry metricnull-in-targets.The check runs on the raw (pre-normalization) YAML, so it also catches scalar-typed nulls (e.g.
workspace.host:) that normalization would otherwise drop silently. The signal is accumulated across all included files viaRoot.Merge.Add acceptance tests in
acceptance/bundle/null/that capture current merge behavior viabundle validate -o json; these will change when null-removes-value merge semantics are introduced in a follow-up PR.This PR was created with GitHub MCP.