PUT on /v2/apps has a PATCH semantic. #5157

aquamatthias · 2017-02-10T12:05:02Z

[EDIT] Add a query parameter, that allows for proper PUT semantics which will overwrite all values.

Add a command line parameter, that allows for proper PUT semantics which will overwrite all values.

aquamatthias · 2017-02-10T12:05:30Z

cc @orlandohohmeier

janisz

Missing test when updateExisting is false.

nLight · 2017-02-10T13:50:36Z

We're happy with the name, thank you for addressing this!

jdef · 2017-02-10T14:20:53Z

docs/docs/rest-api/public/api/v2/apps.raml

@@ -90,6 +90,16 @@
      Note&#58;  This operation will create a deployment. The operation finishes, if the deployment succeeds.
       You can query the deployments endoint to see the status of the deployment.
    is: [ secured, deployable ]
+    queryParameters:
+      updateExisting:


i'm confused by this parameter name. aren't we updating via PUT or PATCH? how does updateExisting imply "I'd like to partially update"?

suggestions: partialUpdate, patchUpdate, applyAsPatch

jdef · 2017-02-10T14:20:59Z

docs/docs/rest-api/public/api/v2/apps.raml

@@ -251,6 +261,16 @@
        Note&#58;  This operation will create a deployment. The operation finishes, if the deployment succeeds.
         You can query the deployments endoint to see the status of the deployment.
      is: [ secured, deployable ]
+      queryParameters:
+        updateExisting:


jdef · 2017-02-10T14:23:05Z

or else, perhaps invert the meaning parameter: call it replaceAll and default it to false?

nLight · 2017-02-10T15:14:35Z

We went ahead and created a tag in docker hub v1.4.0-RC6.5157 then I launched is as MoM and pointed my dcos-ui to it instead of the root Marathon. Then I added the param to the API request, I can see the UI sending it but Marathon still replies with the same error.

aquamatthias · 2017-02-10T15:46:00Z

@nLight
That is a different problem: the problem arises, if the updated appdefinition defines an ipAdress, but no portDefinition.

AppDefinition.empty has a default port
AppDefinition (L92) has a require statement not allowing ipAddress/ports at the same time

@jdef can you have a look?

jdef · 2017-02-10T18:08:17Z

src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala

+        |  },
+        |  "ipAddress": { "name": "dcos" }
+        |}""".stripMargin.getBytes("UTF-8")
+    val appUpdate = appsResource.canonicalAppUpdateFromJson(app.id, body, false)


should we also include a test case where partialUpdate=true to demonstrate that it will fail validation?

jdef · 2017-02-10T18:10:29Z

also discussed with @aquamatthias that we should add support for the PATCH method so that people can migrate their patch-semantic-dependent code to the PATCH API during the 1.4 release cycle so that when we roll out 1.5 we don't break people who are using PUT to get patch-like behavior.

(The plan for 1.5 should be to, at least, change the default of partialUpdate to false)

jdef · 2017-02-10T20:35:45Z

src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala

@@ -141,20 +143,66 @@ class AppsResource @Inject() (
    }
  }

+  /**
+    * Validate and normalize an array of application updates submitted via the REST API. Validation exceptions are not


copy/paste doc error

jdef · 2017-02-10T21:33:42Z

src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala

+      // some hackery here to pass initial JSON parsing.
+      val jsObj = Json.parse(body).as[JsObject] + ("id" -> Json.toJson(appId.toString))
+      // the version is thrown away in toUpdate so just pass `zero` for now
+      normalizedApp(validateOrThrow(jsObj.as[AppDefinition].copy(id = appId).withCanonizedIds()), Timestamp.zero).toUpdate


we don't actually need to copy the id in this step since we just did it in the previous one

jdef · 2017-02-10T21:47:09Z

overall i'm not super happy w/ the efficiency when partialUpdate==false because we basically go from AppDefinition to AppUpdate back to AppDefinition again. but this approach minimizes the changes required for this PR and probably doesn't actually add "too much" overhead for app update operations. if performance concerns crop up in testing we can tackle a larger refactor then

jdef · 2017-02-10T21:48:04Z

GroupDeployIntegrationTest seems to flake out when integration testing on CI but runs just fine locally

[EDIT] I'm guessing it's the 3s timeout for "update a group with application with health checks"

Summary: 1.) PUT on /v2/apps has a PATCH semantic. (#5157) See 8df2a5d * document that, by default, PUT on /v2/apps has a PATCH semantic: only the fields specified in the app update are applied to the existing app definition. * add a query parameter, `partialUpdate`, that allows for proper PUT semantics for an app. `false` means "completely replace the existing app with the new one that I'm **fully specifying** here" 2.) Add support for PATCH updates to apps in 1.4 (#5183) See 1ba8ea7 * added support to PATCH apps also-by: unterstein 3.) Fixes #5211 by defining the jersey annotated methods explicitly. (#5217) See e1b7952 * With the newly introduced `PATCH` semantic, we introduced a misleading method declaration which is not resolvable for jersey. Therefore this logic was restructured and methods are defined explicitly. Test Plan: sbt test sbt integration:test Reviewers: meichstedt, aquamatthias, jasongilanfarr, jenkins, jdef Reviewed By: meichstedt, aquamatthias, jenkins, jdef Subscribers: jdef, marathon-team Differential Revision: https://phabricator.mesosphere.com/D552

PUT on /v2/apps has a PATCH semantic.

56a62f0

Add a command line parameter, that allows for proper PUT semantics which will overwrite all values.

aquamatthias added the in progress label Feb 10, 2017

aquamatthias requested review from meichstedt and jeschkies February 10, 2017 12:05

marcomonaco added sprint-4 and removed sprint-4 labels Feb 10, 2017

timcharper mentioned this pull request Feb 10, 2017

Release Marathon 1.4.0-RC8 #5110

Closed

13 tasks

jeschkies approved these changes Feb 10, 2017

View reviewed changes

janisz suggested changes Feb 10, 2017

View reviewed changes

orlandohohmeier approved these changes Feb 10, 2017

View reviewed changes

meichstedt approved these changes Feb 10, 2017

View reviewed changes

jdef reviewed Feb 10, 2017

View reviewed changes

improve API clarity: rename updateExisting parameter to partialUpdate

aedf113

jdef added this to the Marathon 1.4 milestone Feb 10, 2017

jdef self-assigned this Feb 10, 2017

jdef added API labels Feb 10, 2017

make PUT: partialUpdate=false semantics actually work; added unit test

74dac83

jdef reviewed Feb 10, 2017

View reviewed changes

jdef added 2 commits February 10, 2017 15:07

stop scaladoc from complaining

495ee99

improved unit test

afc76da

jdef reviewed Feb 10, 2017

View reviewed changes

fix another doc error

602a9d7

jdef mentioned this pull request Feb 10, 2017

add support for PATCH because we're deprecating partial-update semantics for PUT in 1.4 #5161

Closed

jdef reviewed Feb 10, 2017

View reviewed changes

remove superfluous copy(id = ...) operation

ca1c2bc

jdef merged commit 8df2a5d into releases/1.4 Feb 10, 2017

jdef removed the in progress label Feb 10, 2017

jdef deleted the mv/put_like_put branch February 10, 2017 21:59

jdef mentioned this pull request Feb 10, 2017

forward-port PATCH related code to master #5162

Closed

janisz mentioned this pull request Feb 25, 2017

Document PATCH-like PUTs #594

Closed

marcomonaco added the pr label Mar 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PUT on /v2/apps has a PATCH semantic. #5157

PUT on /v2/apps has a PATCH semantic. #5157

aquamatthias commented Feb 10, 2017 •

edited by jdef

Loading

aquamatthias commented Feb 10, 2017

janisz left a comment

nLight commented Feb 10, 2017

jdef Feb 10, 2017

jdef Feb 10, 2017

jdef commented Feb 10, 2017

nLight commented Feb 10, 2017

aquamatthias commented Feb 10, 2017

jdef Feb 10, 2017

jdef commented Feb 10, 2017

jdef Feb 10, 2017

jdef Feb 10, 2017

jdef commented Feb 10, 2017

jdef commented Feb 10, 2017 •

edited

Loading

PUT on /v2/apps has a PATCH semantic. #5157

PUT on /v2/apps has a PATCH semantic. #5157

Conversation

aquamatthias commented Feb 10, 2017 • edited by jdef Loading

aquamatthias commented Feb 10, 2017

janisz left a comment

Choose a reason for hiding this comment

nLight commented Feb 10, 2017

jdef Feb 10, 2017

Choose a reason for hiding this comment

jdef Feb 10, 2017

Choose a reason for hiding this comment

jdef commented Feb 10, 2017

nLight commented Feb 10, 2017

aquamatthias commented Feb 10, 2017

jdef Feb 10, 2017

Choose a reason for hiding this comment

jdef commented Feb 10, 2017

jdef Feb 10, 2017

Choose a reason for hiding this comment

jdef Feb 10, 2017

Choose a reason for hiding this comment

jdef commented Feb 10, 2017

jdef commented Feb 10, 2017 • edited Loading

aquamatthias commented Feb 10, 2017 •

edited by jdef

Loading

jdef commented Feb 10, 2017 •

edited

Loading