move parsing key-value files to a separate package #5502
Conversation
thaJeztah
added
kind/enhancement
status/2-code-review
kind/refactor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5502 +/- ##
=======================================
Coverage 60.09% 60.09%
=======================================
Files 345 345
Lines 23441 23448 +7
=======================================
+ Hits 14086 14091 +5
- Misses 8381 8382 +1
- Partials 974 975 +1 |
Move the code for parsing key-value files, such as used for env-files and label-files to a separate package. This allows other projects (such as compose) to use the same parsing logic, but provide custom lookup functions for their situation (which is slightly different). The new package provides utilities for parsing key-value files for either a file or an io.Reader. Most tests for EnvFile were now testing functionality that's already tested in the new package, so were (re)moved. Co-authored-by: Nicolas De Loof <[email protected]> Signed-off-by: Nicolas De Loof <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
separate_kvfile
branch
from
6f44986
to
9ecfe4f
Compare
| // recommended to use a subset of the POSIX portable character set, as | ||
| // outlined in [Environment Variables]. |
There was a problem hiding this comment.
Indeed, I remember having a discussion about this some time ago and while some programs will only accept a subset of the portable character set, there's no guarantees and often applications that people run rely on other random characters, so we shouldn't do much validating here ourselves.
There was a problem hiding this comment.
Yeah, compose recently ran into some of these. And, yes, Linux accepts "just about anything" (anything except NUL terminator). And, just because it does doesn't mean you should ... but some systems do, which is why we don't do validation on any of these.
There was a problem hiding this comment.
It might've been on Compose, yeah 😅
| // UTF16 with BOM | ||
| e := "contains invalid utf8 bytes at line" | ||
| e := "invalid utf8 bytes at line" | ||
| if _, _, _, err := parseRun([]string{"--env-file=testdata/utf16.env", "img", "cmd"}); err == nil || !strings.Contains(err.Error(), e) { | ||
| t.Fatalf("Expected an error with message '%s', got %v", e, err) |
There was a problem hiding this comment.
FWIW; perhaps some of these tests should be moved to the new package as well, but didn't look at that yet.
Move the code for parsing key-value files, such as used for env-files and label-files to a separate package. This allows other projects (such as compose) to use the same parsing logic, but provide custom lookup functions for their situation (which is slightly different).
The new package provides utilities for parsing key-value files for either a file or an io.Reader. Most tests for EnvFile were now testing functionality that's already tested in the new package, so were (re)moved.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)