Improve volume spec parsing #152
Conversation
Codecov Report
@@ Coverage Diff @@
## master #152 +/- ##
==========================================
- Coverage 47.09% 46.98% -0.12%
==========================================
Files 171 171
Lines 11527 11496 -31
==========================================
- Hits 5429 5401 -28
+ Misses 5788 5785 -3
Partials 310 310 |
|
|
||
| buffer := []rune{} | ||
| for _, char := range spec { | ||
| for _, char := range spec + string(endOfSpec) { |
There was a problem hiding this comment.
Interesting. I wonder if it would make sense to iterate over bytes instead of runes, so that we could just check if the iteration count is equal to the length of the sequence minus one. I don't think Windows drive letters can be non-ASCII, but I could be wrong about this.
There was a problem hiding this comment.
That's kind of what the old code did. Instead of using this token it would just repeat this case outside of the for loop. We still need a token to pass to populateFieldFromBuffer() to identify "end of string", so I realized it was actually easier to just append the token to the string and add it to the case.
|
|
||
| expected := len(x.expected) > 1 | ||
| msg := fmt.Sprintf("Case %d: %s", casenumber, x.input) | ||
| assert.Equal(t, expected, parsed.Source != "", msg) |
There was a problem hiding this comment.
I believe you can do the string formatting directly inside the final arguments to assert.Equal. But it's fine if this is a stylistic decision.
There was a problem hiding this comment.
Yup, just a style thing
f0dbbf4 to
da581fe
Compare
vdemeester
left a comment
There was a problem hiding this comment.
LGTM 🐯
Just a nit comment
cli/compose/loader/volume_test.go
Outdated
| assert.False(t, isFilePath("a界")) | ||
| } | ||
|
|
||
| func TestVolumeSplitN(t *testing.T) { |
There was a problem hiding this comment.
Shouldn't this one be renamed 👼
There was a problem hiding this comment.
Ya, I guess this could use a better name. Updated
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Restore testcases for Volume spec parsing. And correctly interpret the parsed volume. Signed-off-by: Daniel Nephin <dnephin@docker.com>
da581fe to
732261f
Compare
[17.07] Fix awslogs driver repeating last event - #34292
We currently have two pieces of code that attempt to parse a volume spec on the client in a platform agnostic way.
One is only a partial parse, the other is a full parse (which is required for Swarm services). Use the full parse for both cases.
Includes some minor improvements to the parsing code.
Note: The code is still in the
cli/compose/loaderpackage. I thought about moving it, but it doesn't seem appropriate. The compose file and cli flags are two different frontends to the same APIs, so I think it's expected they would share code.This code is being split off from #61 since this was a more substantial change, and seemed to warrant further discussion. There is some discussion about this change in that PR. I believe I've addressed the issues.