Remove the experiment flag from output redactor #1452
Conversation
If there is a trailing comma in a list we get empty string, now that will be dropped. This will also return an empty array when specifying a blank string for list fields e.g.: `--redact-vars=` `--redact-vars=''` `--redact-vars ''`
Ensure getValuesToRedact returns something sensible
|
The remaining change to make here is in bootstrap.go |
| if valueAsSlice, ok := value.([]string); ok { | ||
| normalizedSlice := []string{} | ||
|
|
||
| for _, value := range valueAsSlice { | ||
| // Split values with commas into fields | ||
| for _, normalized := range strings.Split(value, ",") { | ||
| if normalized == "" { |
There was a problem hiding this comment.
This change has broader implications beyond the redaction change as it affects the other list type config parameters.
Those fields are:
- Tags
- TagsFromEC2MetaDataPaths
- TagsFromGCPMetaDataPaths
- Experiments
None of those convey semantic meaning to an empty string so I don’t have concerns that this will result in a change in practice.
| @@ -65,3 +65,50 @@ func TestRedactorResetMidStream(t *testing.T) { | |||
| t.Errorf("Redaction failed: %s", buf.String()) | |||
| } | |||
| } | |||
|
|
|||
| func TestRedactorSlowLoris(t *testing.T) { | |||
|
I’ve added the last piece to this which is enabling the redactor code path based on whether there are any vars to redact. I’ve verified that it isn’t enabled when |
There was a problem hiding this comment.
So great to see an experimental feature graduate to general availability 😍
I gave this a go, and confirmed the redaction was enabled without any experiment flags:
Do we definitely want to output the comment into every build log? I guess it might be nice to offer the visible reassurance that redaction is enabled?
Good point, I don’t think we should print our own stuff during the command step. I’ve conditionalised this logging on whether
|
This will remove the
output-redactorexperiment flag and enable it all the time.I have read through the redactor source code and added a couple more comments to help me understand it. It appears sound to me. I identified one issue which I added a test for to verify where if two needles share a prefix the shortest one will be redacted while the tail of the longer will be printed. This seems unlikely to occur in practice.
I have also ensured support for an empty list of redacted vars throughout so that (once the flag is changed) a lack of vars to redact will act as the switch to disable the
Redactorbeing added to the log stream processing. This gives us an escape hatch should there be any issues.Updated docs are in buildkite/docs#1073