Fixes #3723 - Fix validation of duplicate volume names #3737
Conversation
| g.transitiveApps | ||
| .flatMap { app => namesOfMatchingVolumes(app).map(_ -> app.id) } | ||
| .groupBy { case (volumeName, _) => volumeName } | ||
| .mapValues(_.map { case (volumeName, appId) => appId }) | ||
|
|
||
| val groupViolations = g.apps.flatMap { app => |
There was a problem hiding this comment.
The g.apps instead of g.transitiveApps prevented validation of apps in nested groups.
by not using acronyms and by using named parameters
kolloch
force-pushed
the
pk/fix-3723-volume-validation
branch
from
4e85d73
to
235e799
Compare
kolloch
force-pushed
the
pk/fix-3723-volume-validation
branch
from
235e799
to
8795607
Compare
| } | ||
|
|
||
| validator[AppDefinition] { app => | ||
| app.externalVolumes is every(volumeNameUnique(app.id)) |
There was a problem hiding this comment.
To have the correct path here, you can do:
app.externalVolumes as "container/volumes" is every(volumeNameUnique(app.id))There was a problem hiding this comment.
Then the index is wrong
Matthias Veit notifications@github.com schrieb am Di., 12. Apr. 2016
08:12:
In
src/main/scala/mesosphere/marathon/core/externalvolume/impl/providers/DVDIProvider.scala
#3737 (comment):
if otherApp != app.id // do not compare to self} yield RuleViolation(app.id, s"Requested volume $name conflicts with a volume in app $otherApp", None)val appValid: Validator[AppDefinition] = {def volumeNameUnique(appId: PathId): Validator[ExternalVolume] = {def conflictingApps(vol: ExternalVolume): Set[PathId] =appsByVolume.getOrElse(vol.external.name, Set.empty).filter(_ != appId)
isTrue { (vol: ExternalVolume) =>val conflictingAppIds = conflictingApps(vol).mkString(", ")s"Volume name '${vol.external.name}' in $appId conflicts with volume(s) of same name in app(s): " +s"$conflictingAppIds"}{ vol => conflictingApps(vol).isEmpty }}
validator[AppDefinition] { app =>app.externalVolumes is every(volumeNameUnique(app.id))To have the correct path here, you can do:
app.externalVolumes as "container/volumes" is every(volumeNameUnique(app.id))
—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/mesosphere/marathon/pull/3737/files/8795607fb366bab5b1537cf741f15c6478c3aede#r59325005
|
|
||
| isTrue { (vol: ExternalVolume) => | ||
| val conflictingAppIds = conflictingApps(vol).mkString(", ") | ||
| s"Volume name '${vol.external.name}' in $appId conflicts with volume(s) of same name in app(s): " + |
There was a problem hiding this comment.
Let's fix the path, and make the description simple.
The conflictingApps function can be moved inside the isTrue block.
There was a problem hiding this comment.
I am not sure what is best there. I'll keep it "as is". We can improve it later.
|
@kolloch added some suggestions but generally looks good. |
|
Thanks! |
| ) | ||
| } | ||
|
|
||
| class Fixture { |
There was a problem hiding this comment.
why is this a class instead of an object?
There was a problem hiding this comment.
We use a similar pattern elsewhere. If your Fixture contains state, it is nice to use a new instance for every test run.
No description provided.