HomeMesosphereNo notifications. 4 unresolved issues.

putGroup with nested groups now correctly handles transitive apps
ClosedAll Users

Authored by zen-dog on Jun 19 2017, 5:05 PM.

Details

Summary

Fix a bug where transitiveApp weren't correctly calculated for certain combination of nested groups. Added new tests to cover this and similar use cases

Fixes: MARATHON-7433

Test Plan

unit

Diff Detail

Repository
rMARATHON marathon
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Changes from before your most recent comment are hidden. Show Older Changes
jeschkies accepted this revision.Jun 20 2017, 11:58 AM

Thanks. I'm wondering if we should cover more combinations.

src/main/scala/mesosphere/marathon/api/v2/GroupsResource.scala
316

Is versionChange triggering the rollback?

src/test/scala/mesosphere/marathon/state/RootGroupTest.scala
507

What about the case

val groupUpdate = createGroup(
      PathId("/domain"),
      groups = Set(createGroup(
        PathId("/domain/developers"),
        groups = Set(
          createGroup(
            PathId("developers/gitlab"),
apps = Map(appPath -> app)))) ))

This should work as well, right?

What do you think about a loop or table cross different combinations?

val groups =
Table(("first",        "second",                       "third",                                      "appId"),
          ("/domain", "/domain/developers", "/domain/developers/gitlab", "/domain/developers/gitlab/git"),
          ("/domain", "developers",                 "/domain/developers/gitlab", "/domain/developers/gitlab/git"),
          (...)
 forAll (groups) { (first: String, seconds ....) =>
 }

or

val firstLevels = Seq("/domain", "/domain/developers")
val secondLevels = Seq("/domain/developers", "developers")
val thirdLevels = Seq(...)
for {
  first <- firstLevels,
  seconds <- ....
  third ...
} yield {
    //test code ...
 }
zen-dog updated this revision to Diff 3567.Jun 20 2017, 2:53 PM

Rebased

jenkins accepted this revision.Jun 20 2017, 3:08 PM
This revision is now accepted and ready to land.Jun 20 2017, 3:08 PM
✔ Build of 3567 completed jenkins-public-marathon-phabricator-298.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://downloads.mesosphere.io/marathon/snapshots/marathon-1.5.0-SNAPSHOT-593-gdafa95b.tgz",
"sha1"" "5968b9bb8e1540eacb80331399d522a68e1ea1f7"

\\ ٩( ᐛ )و //

zen-dog added inline comments.Jun 20 2017, 3:53 PM
src/test/scala/mesosphere/marathon/state/RootGroupTest.scala
507

What about the case ...

No, you're mixing absolute paths /domain/developers with absolute but nested path developers/gitlab and validation will decline that.

What do you think about a loop or table cross different combinations?

saves some lines but doesn't make readable. I choose more line ;)

This revision was automatically updated to reflect the committed changes.
zen-dog added inline comments.Jun 20 2017, 4:19 PM
src/test/scala/mesosphere/marathon/state/RootGroupTest.scala
507

I meant "relative but nested"