-
Notifications
You must be signed in to change notification settings - Fork 302
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
Fixes #11: Error attempting to ecs-cli compose up #201
Conversation
@@ -48,6 +48,11 @@ var supportedComposeYamlOptions = []string{ | |||
|
|||
var supportedComposeYamlOptionsMap = getSupportedComposeYamlOptionsMap() | |||
|
|||
type volumes struct { | |||
volumeHasHost map[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
volumeHasHost
seems like something that you'd use for naming a bool
. Can you use a better name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
) | ||
|
||
var emptyHostCtr = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be a global variable I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try again. Wherever I had put it before resulted in it overwriting itself every time it was called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically I think I would just have to add another parameter to the function being called otherwise. Should I do that instead?
if sourceVolume != *output.SourceVolume { | ||
t.Errorf("Expected sourceVolume [%s] But was [%s]", sourceVolume, *output.SourceVolume) | ||
} | ||
if readonly != *output.ReadOnly { | ||
t.Errorf("Expected readonly [%s] But was [%s]", readonly, *output.ReadOnly) | ||
t.Errorf("Expected readonly [%v] But was [%v]", readonly, *output.ReadOnly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using testify/assert
package for these kinds of things? Makes it less verbose and more elegant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't because that would require rewriting the entire test file or just changing the section I was dealing with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding testify/assert will be a separate PR since it's not specific to this issue and isn't currently used in the file.
if hostPath != "" { | ||
sourceVolume = volumes.volumeWithHost[hostPath] | ||
} else { | ||
sourceVolume = volumes.volumeEmptyHost[*emptyHostCtr] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that hard-coding this index might be better than incrementing using a pointer. We already hardcode the index for the mountPointsOut
slice when this is invoked. What do you think about not incrementing this variable, but passing in a hard-coded value from where it's invoked? So it'd be verifyMountPoint(t *testing.T, output *ecs.MountPoint, volumes *volumes, hostPath, containerPath string, readonly bool, emptyHostIndex int)
. And then you'll invoke it with the appropriate index from TestConvertToMountPoints()
. Does that make sense?
9c9efe8
to
71bc850
Compare
Fixes #11
-Cherry-picked PR #23 (thanks @mcanevet)
-The above PR made it so it wouldn't fail, but the same volume was shared amongst all container paths without a host path, so I added this
-Updated test
-Tested it with
ecs-cli compose up
on random compose file I mushed together: