-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove yaml.v3 rewrite #91
Comments
This is not possible since the path defined in the yaml fork go.mod will be different than the one imported meaning that we will always require the replace until the feature is upstream |
The reason we are using that fork is because upstream yaml didn't seem to allow us to properly handle unicode chars. From the original issue
To fix that we created a fork and proposed the following changes upstream The actual changes performed in the yamlops library to leverage the forked/extended yaml implementations were --- a/yamlops/yamlops.go
+++ b/yamlops/yamlops.go
@@ -1,6 +1,7 @@
package yamlops
import (
+ "bufio"
"bytes"
"errors"
"fmt"
@@ -100,16 +101,30 @@ func UpdateMap(doc []byte, pathSpec string, contains, values map[string]string)
}
}
+ // XXX the replacement loop below originally relied on the Node's Index &
+ // EndIndex (populated from gopkg.in/yaml.v3's private yaml_mark_t type).
+ //
+ // Unfortunately, Index/EndIndex are affected by unicode chars that occur
+ // before the replacement location.
+ //
+ // Fortunately, Line/Column and EndLine/EndColumn are correct. To use those
+ // we simply need to know the byte-position of the start of each line in the
+ // yaml doc.
+ lines, err := indexLines(doc)
+ if err != nil {
+ return nil, err
+ }
+
// Apply replacements in reverse location order to avoid affecting the
// location of earlier content.
sort.Sort(sort.Reverse(sortValueReplacementByNodeIndex(valueReplacements)))
for _, replacement := range valueReplacements {
node, value := replacement.node, replacement.value
- start := node.Index
- end := node.EndIndex
+ // TODO(mgoodall): this relies on a forked go-yaml.v3 that adds EndLine
+ // & EndColumn.
+ start := lines[node.Line-1] + node.Column - 1
+ end := lines[node.EndLine-1] + node.EndColumn - 1
// Replace value's content in the doc with the new value.
log.Printf("replacing value %q from l%d:%d to l%d:%d with %q\n", node.Value, node.Line, node.Column, node.EndLine, node.EndColumn, value)
@@ -119,6 +134,44 @@ func UpdateMap(doc []byte, pathSpec string, contains, values map[string]string)
return doc, nil
}
+// scanNewLine is a copy of `bufio.ScanLines` that does not strip any '\r'.
+func scanNewLine(data []byte, atEOF bool) (advance int, token []byte, err error) {
+ if atEOF && len(data) == 0 {
+ return 0, nil, nil
+ }
+ if i := bytes.IndexByte(data, '\n'); i >= 0 {
+ // We have a full newline-terminated line.
+ return i + 1, data[0:i], nil
+ }
+ // If we're at EOF, we have a final, non-terminated line. Return it.
+ if atEOF {
+ return len(data), data, nil
+ }
+ // Request more data.
+ return 0, nil, nil
+}
+
+// indexLines returns the 0-based index of the start of each line from the
+// beginning of the doc.
+func indexLines(doc []byte) ([]int, error) {
+ index := []int{}
+
+ scanner := bufio.NewScanner(bytes.NewBuffer(doc))
+ scanner.Split(scanNewLine)
+
+ offset := 0
+ for scanner.Scan() {
+ index = append(index, offset)
+ offset += len(scanner.Bytes()) + 1
+ }
+
+ if err := scanner.Err(); err != nil {
+ return nil, err
+ }
+
+ return index, nil
+}
+ Our alternative here would be to either a) push for the change to be merged upstream or b) figure out an alternative solution |
Currently we have the following rewrite due to some extra functionality required by our yamlops libary that is (or at least was) missing in upstream yaml
We want to get rid of that rewrite and by in order of preference either
1 - Start using yaml.v3 which will require undetermined changes in yamlops
2 - Start using the fork directly in our code not relying on go-mod rewriting, we should also consider creating our own fork
Thoughts @petewall @josvazg @tompizmor
The text was updated successfully, but these errors were encountered: