Run tests, launch an app with "container.docker.config" set
Details
- Reviewers
jenkins meichstedt aquamatthias zen-dog kensipe unterstein jdef jeschkies - Commits
- rMARATHON9450c8e37911: More of testing and validating Docker Pull Config API
rMARATHONa30d8c678ead: Address @aquamatthias' comments
rMARATHON359bae165365: Document Docker image config option
rMARATHON1ee454c46811: Add support of Docker Image config field for pods too
rMARATHON4896abb92cc5: Introduce ImagePullConfig proto message
rMARATHONe43d34540e72: Make sure that config option is used only with both Mesos Containerizer and…
rMARATHON78bb703eeeba: Introduce ImagePullConfig proto message
rMARATHON34bf51bd508e: Support passing a Docker config for task / pod being launched
rMARATHONde2e9bbd060a: Address @jdef's comments
rMARATHONa3c4dab98dd4: Address @jdef's comments
rMARATHONf452d7149626: Address @jdef's comments and use the existing secret interface instead of…
rMARATHONd66c2c00541e: Support passing a Docker config for task / pod being launched
rMARATHONc6bbd6576e5a: Document Docker image config option
rMARATHON4ac015623b5d: Uncomment a plugin
rMARATHON41bf6e4c7e70: Fix a test
rMARATHON37f1068e05f8: Compile *.proto files with protoc v2.6.1
rMARATHON01acfd8593f3: Support container.docker.config.{text,secret}
rMARATHONcc9e43d9b5d8: SI test for a pod pulling an image from a private registry
rMARATHONf6b6cdf5ea8d: SI test for a pod pulling an image from a private registry
rMARATHON32756ea6909a: Fix a test
rMARATHON05ad39208c76: Tell people use protoc v2.6.1
rMARATHON825d33d43e88: Compile *.proto files with protoc v2.6.1
rMARATHON8a2472d3cbcb: Update integration tests to test fetching of private Docker images
rMARATHONe6b229eb08ec: Address comments
rMARATHONdb79c855b81f: Copy mesos.proto from Mesos' master branch (commit…
rMARATHON5d7dfd6613ec: Update integration tests to test fetching of private Docker images
rMARATHONe31af6c1d61d: Address comments for docs
rMARATHONb97782ca9309: Support passing a Docker config.json for app / pod being launched
rMARATHON58363801d813: Drop support for "docker.config.text" and use SecretDef for "docker.config.
rMARATHONea89b203b5b8: Tell people use protoc v2.6.1
rMARATHON1a0540fac9ab: Uncomment a plugin
rMARATHON4013d6404a35: Address comments for docs
rMARATHON39bec377ba9c: Drop support for "docker.config.text" and use SecretDef for "docker.config.
rMARATHONb0e91b9ab642: Make sure that config option is used only with both Mesos Containerizer and…
rMARATHONe5e3817775c5: Replace config with pullConfig in native-docker.md
rMARATHON7795b89a8ecb: Use Mesos 1.4.0-SNAPSHOT
rMARATHON297c502ed930: Support passing a Docker config.json for app / pod being launched
rMARATHONe994b756a437: Support passing a Docker config.json for app / pod being launched
rMARATHON2d1c9c32e46d: SI test for a pod pulling an image from a private registry
rMARATHONca27cb5a7127: Address comments for docs
rMARATHONb0ca2be54a47: More of testing and validating Docker Pull Config API
rMARATHONeb0fd4b85d09: Use Mesos 1.4.0-SNAPSHOT
rMARATHON9d0901eb3402: Support passing a Docker config.json for app / pod being launched
rMARATHON58908feb732d: Address @jdef's comments
rMARATHON5c6cc18e6a2c: Add support of Docker Image config field for pods too
rMARATHONf3e890b0d545: Address @jdef's comments
rMARATHONebe73d447582: Support container.docker.config.{text,secret}
rMARATHON0e5d61d96a8f: Use Mesos 1.3.0-rc3
rMARATHON487e006cbe26: Address @aquamatthias' comments
rMARATHON7d49bc18164b: Bring container.docker.credential back to life, but deprecate it
rMARATHONb37a5c26b3b0: Address @jdef's comments and use the existing secret interface instead of…
rMARATHON084b07b1df64: Bring container.docker.credential back to life, but deprecate it
rMARATHON4b0523692730: Copy mesos.proto from Mesos' master branch (commit… - JIRA Issues
- JIRA MARATHON-7090 UCR Authentication per task / docker registry
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.
✔ Build of 3328 completed jenkins-public-marathon-phabricator-87.
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-538-g44f7e23.tgz", "sha1"" "67f4f9849ae3b340c7aa357d8c2d9363904bc987"
✔ Build of 3340 completed jenkins-public-marathon-phabricator-101.
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-538-g44f7e23.tgz", "sha1"" "d00b47ad3634bac6475ff56b2f62354f0cd95100"
✔ Build of 3341 completed jenkins-public-marathon-phabricator-102.
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-538-g44f7e23.tgz", "sha1"" "cd497bd39061b5d0667d76ff998bcc5f1d073502"
✗ Build of 3342 failed jenkins-public-marathon-phabricator-103.
Error message:
Stage Compile and Test failed.
docs/docs/rest-api/public/api/v2/types/appContainer.raml | ||
---|---|---|
15 | ??? | |
119 | i'm looking for "Deprecated in favor of pullConfig" | |
project/Dependencies.scala | ||
86 | I've heard that there's a 1.3.0-rc1 that contains what we need? | |
src/main/scala/mesosphere/marathon/api/serialization/ContainerSerializer.scala | ||
341 | nit: should not need new here | |
src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala | ||
55 | i don't see a validation test case for this. is it there and missed it? if not, please add one | |
src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala | ||
134 | this case _ is superfluous since ImageType is a sealed trait | |
158 | wow, we really left abc in there, eh? | |
src/main/scala/mesosphere/marathon/raml/ContainerConversion.scala | ||
200 | ditto, new is not required here | |
src/main/scala/mesosphere/mesos/TaskGroupBuilder.scala | ||
350 ↗ | (On Diff #3342) | nit: eliminate this added newline and this file drops from the diff |
src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala | ||
168 | happy to see some tests here. i'd like a more thorough exercise of the validation rules to appear in AppValidationTest | |
src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala | ||
240 | happy to see some tests here. i'd like a more thorough exercise of the validation rules to appear in PodsValidationTest |
Some minor changes but otherwise lgtm. Thx!
tests/system/common.py | ||
---|---|---|
875–876 | It seems like next 10 lines can be replaced by create_docker_pull_config_json() call? | |
tests/system/marathon_common_tests.py | ||
986–987 | Those asserts here and in test_create_pod_with_private_image() are not necessary since you skip the test if they're not there anyway? |
tests/system/common.py | ||
---|---|---|
472–482 | I don't know what "private_mesos_app" means... can we rename this to something meaningful? or add docs that explains why it should be used? | |
865 | this is in shakedown... all of this can be replaced with shakedown. distribute_docker_credentials_to_private_agents() | |
tests/system/marathon_common_tests.py | ||
986–987 | I don't see docker_env_set() in the skipif.. however it is likely that we are skipping if we are missing expected ENV.. that makes these assert 'DOCKER_XYZ` not useful. | |
tests/system/marathon_pods_tests.py | ||
95 | do we need these assertions? | |
110 | why do we define the app json for secrets behind a function and for the longer pod json we do not? |
tests/system/common.py | ||
---|---|---|
472–482 | App id should have a random part (see using of uuid in other tests). Prevents collisions and makes debugging easier. | |
865 | This is not the same - there is no need to distribute the file, since it's passed as part of the app definition. However we should extract creation part into a separate function and maybe move it to shakedown later. |
tests/system/common.py | ||
---|---|---|
472–482 | It was meant as an addition to your comment ;) |
✔ Build of 3356 completed jenkins-public-marathon-phabricator-114.
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-542-gba55703.tgz", "sha1"" "d1fa8e7e2d01c05830129043c4cdaf3e833dc322"
✔ Build of 3357 completed jenkins-public-marathon-phabricator-115.
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-542-gba55703.tgz", "sha1"" "abe173ddd939a9eccadf32115f50264b9e042b9e"
wow, a bunch of feedback already given and addressed.
On top I would add only one nit.
Except open feedback LGTM 🚀
Accept in advance
docs/docs/pods.md | ||
---|---|---|
184 | s/config/pullConfig/ |
✔ Build of 3377 completed jenkins-public-marathon-phabricator-134.
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-542-gba55703.tgz", "sha1"" "56551544aaf0dc78a352f40840078fea5255805d"
I've left ??? to indicate unanswered questions from prior reviews
docs/docs/rest-api/public/api/v2/types/appContainer.raml | ||
---|---|---|
15 | ??? | |
119 | ??? | |
docs/docs/rest-api/public/api/v2/types/docker.raml | ||
12 | nit: required: true is redundant (because it's the default). i don't think we say that many other places? | |
src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala | ||
127 | Pretty sure that image.id validation is already handled by the generated RAML code: val id = json.\("id").validate[String](play.api.libs.json.JsPath.read[String](maxLength[String](1024) keepAnd minLength[String](1))) so .. no need to duplicate that validation here. the length check above (in the dockerImageValidator) code is redundant, and there's already a JIRA filed for removing redundant validation. we shouldn't add any more |
✗ Build of 3380 failed jenkins-public-marathon-phabricator-140.
Error message:
Stage Compile and Test failed.
It'd be pretty sweet if the RAML DockerCredentials type had a deprecation notice somewhere:
DockerCredentials: type: object description: Credential to authenticate with the docker registry properties: principal: type: string description: Principal to authenticate with the docker registry secret?: type: string description: Secret to authenticate with the docker registry
✗ Build of 3382 failed jenkins-public-marathon-phabricator-141.
Error message:
Stage Compile and Test failed.
nit: s/follow/following/