-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Fix index out of range on compose.buildContainerMountOptions
#8750
Conversation
l := len(s.Volumes) - 1 | ||
s.Volumes[i] = s.Volumes[l] | ||
s.Volumes = s.Volumes[:l] |
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 is an attempt at deleting the i element, but clearly doing this while iterating is a recipe for disaster…
However there was a definite intent there, as the comment seems to imply.
Do we really not know what it’s supposed to do?
Do we have unit tests? 😁
@@ -762,10 +762,6 @@ func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img moby | |||
Target: m.Destination, | |||
ReadOnly: !m.RW, | |||
} | |||
// Avoid mount to be later re-defined |
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'm not sure I remember the detail, but deletion here is expected afaict.
Maybe better create a new Volumes
before the loop, copy those that need to be kept while iterating, then override s.Volumes
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.
Yes, either that or a good old for i := 0; i < len(s.Volumes {
, incrementing i only when not deleting.
I actually removed that cuz in the end in the |
a93f66b
to
479d3e4
Compare
Signed-off-by: Ulysses Souza <[email protected]>
479d3e4
to
44c5e5d
Compare
What I did
Remove buggy code, since it seems to be incorrect even when not panic-ing
Related issue
Resolves #8505