accepting wildcard for ping to enable previous behavior for 1.4
ClosedAll Users
Actions

Authored by kensipe on Jun 29 2017, 8:55 PM.

Details

Reviewers

zen-dog
jenkins
jeschkies
timcharper
ichernetsky
ajain13
jdef

JIRA Issues

JIRA MARATHON-7566 Marathon 1.5 Ping Does NOT provide default content

Test Plan

run si test

Diff Detail

Repository

rMARATHON marathon

Branch

ken/wild-ping-1.4

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 3026
Build 5756: Marathon (revised)	Jenkins
Build 5755: arc lint + arc unit

kensipe created this revision.Jun 29 2017, 8:55 PM

Herald added a subscriber: marathon-dev. · View Herald TranscriptJun 29 2017, 8:55 PM

jenkins requested changes to this revision.Jun 29 2017, 8:55 PM

This revision now requires changes to proceed.Jun 29 2017, 8:55 PM

kensipe added reviewers: jeschkies, timcharper, ichernetsky, ajain13, jdef.Jun 29 2017, 8:56 PM

ichernetsky accepted this revision.Jun 29 2017, 8:59 PM

jenkins accepted this revision.Jun 29 2017, 9:09 PM

This revision is now accepted and ready to land.Jun 29 2017, 9:09 PM

✔ Build of 3675 completed jenkins-public-marathon-phabricator-392.

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-627-gc392203.tgz",
"sha1": "1152f2b4b4e3ba22f6875e8cb59541189048f9dd"

＼\ ٩( ᐛ )و /／

Harbormaster completed building B3026: Diff 3675.Jun 29 2017, 9:09 PM

ajain13 accepted this revision.Jun 30 2017, 3:12 AM

kensipe closed this revision.Jun 30 2017, 6:50 PM

We might want a change log entry for this

src/main/scala/mesosphere/marathon/api/SystemResource.scala
40	I think this is a breaking change. We had a customer whose ping broke: https://jira.mesosphere.com/browse/MARATHON_EE-1522. Would wildcard work them?

I made assumptions in my initial review that appear to be false.

I spent some time this morning refreshing on http content negotiation and the use of wildcard. I can't actually find any valid use case for a server returning a wildcard content type, only that clients may accept wildcard. A server could add a "Vary: *" header to mitigate caching, but that's independent of content type.

We might actually want to revert this change since, upon second look, it appears harmful because it's outside the http spec.

jdef mentioned this in D911: ping: provide a content-type that is better aligned with client expectations.Jul 7 2017, 12:30 AM

Revision Contents

			Path	Packages
M			src/main/scala/mesosphere/marathon/api/SystemResource.scala (2 lines)

Commit	Tree	Parents	Author	Summary	Date
c8ab4bbf4197	9ce6c6c3083c	c3922031bb69	Ken Sipe	accepting wildcard for ping to enable previous behavior for 1.4 (Show More…)	Jun 29 2017, 8:53 PM

Diff 3675

View Options

src/main/scala/mesosphere/marathon/api/SystemResource.scala

Show All 31 Lines
32	32		@Produces(Array(MarathonMediaType.PREFERRED_APPLICATION_JSON))
33	33		class SystemResource @Inject() (val config: MarathonConf, cfg: Config)(implicit
34	34		val authenticator: Authenticator,
35	35		val authorizer: Authorizer,
36	36		actorSystem: ActorSystem) extends RestResource with AuthResource with StrictLogging {
37	37
38	38		@GET
39	39		@Path("ping")
40			@Produces(Array(MediaType.TEXT_PLAIN))
	40		@Produces(Array(MediaType.WILDCARD))
		jeschkiesUnsubmitted Not Done I think this is a breaking change. We had a customer whose ping broke: https://jira.mesosphere.com/browse/MARATHON_EE-1522. Would wildcard work them?
41	41		def ping(): Response = ok("pong")
42	42
43	43		@GET
44	44		@Path("metrics")
45	45		def metrics(@Context req: HttpServletRequest): Response = authenticated(req) { implicit identity =>
46	46		withAuthorization(ViewResource, SystemMetrics){
47	47		ok(jsonString(Raml.toRaml(Metrics.snapshot())))
48	48		}
▲ Show 20 Lines • Show All 57 Lines • Show Last 20 Lines

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	3675	c392203		Jun 29 2017, 8:55 PM	★	★