Implement package build command #808

larioj · 2016-10-13T17:00:02Z

This PR is part of the new packaging workflow that is currently in development. Specifically this is to satisfy milestone 1 of the packaging flow. See document https://docs.google.com/document/d/1VEx2O28Aiiv-JgIBrdoWhD3gTAYfbz0D4JLUOhnwdPY/edit?usp=sharing for a more complete discussion of the new flow.

A quick rundown of what this PR will add to the CLI:

$ dcos package build build_definition.json
Created DC/OS Universe package [<package-name>-<version>-<md5-hash>.dcos].

The CLI will resolve any local references found in build-definition.json to create the bundle.

jsancio

LGTM! Minor changes.

Can you please add example outputs from using this new command in the comments.

jsancio · 2016-10-13T18:55:45Z

cli/dcoscli/data/help/package.txt


 Commands:
+    bundle
+        Bundle a package for dcos consumption.


I would change this to "Bundle a package to install to DC/OS or share with Universe."

jsancio · 2016-10-13T18:59:35Z

cli/dcoscli/data/schemas/package-schema.json

+          "description": "[Deprecated v3.x] An array of strings representing of the requirements file to use for installing the subcommand for Pip. Each item is interpreted as a line in the requirements file."
+        }
+      }
+    },


I need to follow up on this but I would like to get rid of this. It will be hard to make packages self-contain if we support this.

Actually, @tamarrow what do you think about get rid of this? This means that we can never generate DC/OS Package that are have a Python CLI.

jsancio · 2016-10-13T19:16:42Z

cli/dcoscli/package/main.py

+            '{}-{}-{}.dcos'.format(
+                package_resolved['name'],
+                package_resolved['version'],
+                sha256_file(temp_file.name)))


We changed the algorithm to use MD5 so that the hash and filename is smaller.

jsancio · 2016-10-13T19:24:40Z

cli/dcoscli/package/main.py

+
+
+def _resolve_local_references(package_json, package_directory):
+    """ Resolves all local refereces in package json


You need a newline after this line. Our doc generator needs this.

jsancio · 2016-10-13T19:28:30Z

cli/dcoscli/package/main.py

+    """
+    bundle_schema_path = "data/schemas/bundle-schema.json"
+    bundle_schema = util.load_jsons(
+        pkg_resources.resource_string("dcoscli", bundle_schema_path).decode("utf-8"))


Instead of loading it from disk, I would pass the bundle_schema in.

What do you mean?

Where do I pass it in from? How do I get it to begin with?

As a parameter to this function.

For example you already constructed this value from the function that calls this function.

Ohhhh, I see what you mean.

jsancio · 2016-10-13T19:30:05Z

cli/dcoscli/package/main.py

+   :rtype: bool
+   """
+    local_ref_pattern = re.compile("^@")
+    return isinstance(item, str) and local_ref_pattern.match(item)


We don't need to use regex. You just need to know that the string to start with @.

jsancio · 2016-10-13T19:36:32Z

cli/tests/integrations/test_bundle.py

+
+    # check that the output is correct
+    assert return_code == 1
+    assert stderr == expected_error


I would also assert that no files where created in the temp folder.

jsancio · 2016-10-13T19:39:01Z

cli/tests/integrations/test_bundle.py

+def test_package_missing_references():
+    _failure_test("tests/data/bundle/package_missing_references.json",
+                  b"Error opening file [/Users/mesosphere"
+                  b"/Work/dcos-cli/cli/tests/data/bundle/marathon.json]: No such file or directory\n")


Full path won't work in other environment like the CI.

jsancio · 2016-10-13T19:39:13Z

cli/tests/integrations/test_bundle.py

+    _failure_test("tests/data/bundle/package_reference_does_not_match_schema.json",
+                  b"Error validating package: "
+                  b"[/Users/mesosphere/Work/dcos-cli/cli/tests/data/bundle/resource-bad.json] "
+                  b"does not conform to the specified schema\n")


Full path won't work in other environment like the CI.

jsancio · 2016-10-13T19:42:30Z

dcos/util.py

+    with open(filename, 'rb') as f:
+        for chunk in iter(lambda: f.read(4096), b''):
+            hasher.update(chunk)
+    return hasher.hexdigest()


We change the format to use MD5 because it is a smaller string in the filename.

larioj · 2016-10-13T22:40:53Z

@jsancio Which comments. The Github comments or the comments in the code?

jsancio · 2016-10-13T23:05:23Z

GitHub PR comment. For new commands it is nice for the reviewer to be able to see how the command works by seeing some examples.

jsancio · 2016-10-16T00:56:24Z

@cruhland Was this closed by accident?

jsancio · 2016-10-16T00:58:07Z

cli/dcoscli/data/schemas/bundle-schema.json

+          "type": "boolean",
+          "description": "Flag indicating if the package is selected in search results",
+          "default": false
+        },


selected is not allowed.

jsancio · 2016-10-16T00:58:38Z

cli/dcoscli/data/schemas/bundle-schema.json

+          "type": "boolean",
+          "description": "Flag indicating if the package is selected in search results",
+          "default": false
+        },


selected is not allowed.

jsancio · 2016-10-16T00:59:54Z

cli/dcoscli/data/schemas/bundle-schema.json

+          "type": "integer",
+          "description": "Corresponds to the revision index from the universe directory structure",
+          "minimum": 0
+        },


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:00:23Z

cli/dcoscli/data/schemas/package-schema.json

+          "type": "boolean",
+          "description": "Flag indicating if the package is selected in search results",
+          "default": false
+        },


selected is not allowed.

jsancio · 2016-10-16T01:00:32Z

cli/dcoscli/data/schemas/package-schema.json

+          "type": "boolean",
+          "description": "Flag indicating if the package is selected in search results",
+          "default": false
+        },


selected is not allowed.

jsancio · 2016-10-16T01:01:47Z

cli/dcoscli/data/schemas/bundle-schema.json

+          "type": "integer",
+          "description": "Corresponds to the revision index from the universe directory structure",
+          "minimum": 0
+        },


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:02:05Z

cli/dcoscli/data/schemas/bundle-schema.json

+        "packagingVersion",
+        "name",
+        "version",
+        "releaseVersion",


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:02:35Z

cli/dcoscli/data/schemas/package-schema.json

+          "type": "integer",
+          "description": "Corresponds to the revision index from the universe directory structure",
+          "minimum": 0
+        },


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:02:47Z

cli/dcoscli/data/schemas/package-schema.json

+          "type": "integer",
+          "description": "Corresponds to the revision index from the universe directory structure",
+          "minimum": 0
+        },


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:02:56Z

cli/dcoscli/data/schemas/package-schema.json

+        "packagingVersion",
+        "name",
+        "version",
+        "releaseVersion",


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:03:10Z

cli/tests/data/bundle/package_all_references.json

+  "packagingVersion": "3.0",
+  "name": "bitbucket",
+  "version": "4.5",
+  "releaseVersion": 3,


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:03:18Z

cli/tests/data/bundle/package_config_reference.json

+  "packagingVersion": "3.0",
+  "name": "bitbucket",
+  "version": "4.5",
+  "releaseVersion": 3,


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:03:28Z

cli/tests/data/bundle/package_config_reference_expected.json

+  "packagingVersion": "3.0",
+  "name": "bitbucket",
+  "version": "4.5",
+  "releaseVersion": 3,


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:03:39Z

cli/tests/data/bundle/package_marathon_reference.json

+  "packagingVersion": "3.0",
+  "name": "bitbucket",
+  "version": "4.5",
+  "releaseVersion": 3,


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

jsancio · 2016-10-16T01:03:46Z

cli/tests/data/bundle/package_marathon_reference_expected.json

+  "packagingVersion": "3.0",
+  "name": "bitbucket",
+  "version": "4.5",
+  "releaseVersion": 3,


We should remove this property from the bundle-schema and the package-schema. This is controlled by Universe and Cosmos.

tamarrow-zz · 2016-10-17T22:00:58Z

cli/dcoscli/package/main.py

+                                ' specified schema'.format(location))
+
+
+def _replace_marathon(bundle_schema, package_directory, package_json):


missing pydoc

the _replace_X methods are all very similar. lets use a shared method and call it for each different file or "ref"

tamarrow-zz · 2016-10-17T22:02:29Z

cli/tests/data/dcos.toml

@@ -1,5 +1,5 @@
 [core]
+timeout = 5


this should be reverted

tamarrow-zz · 2016-10-17T22:03:25Z

cli/tests/data/help/package.txt


 Positional Arguments:
+    <package-json>
+        Path to the package json of the project.


what is 'package-json'? Is this package.json in universe or package manifest? We should link to a schema ref.

tamarrow-zz · 2016-10-17T22:03:42Z

setup.py


    install_requires=[
-        'jsonschema==2.4',  # pin the exact version, jsonschema 2.5 broke py3
+        'jsonschema>=2.5',


did we verify this on 2.7?

@tamarrow working on it.

@tamarrow Jose and I verified that it works.

tamarrow-zz · 2016-10-17T22:05:30Z

dcos/util.py

+
+
+def hash_file(filename):
+    """Calculates the sha256 of a file


dcos.subcommand has a _hashfile method for sha256. Looks like this is md5, we should name the function as such

tamarrow-zz · 2016-10-17T22:08:00Z

cli/tests/integrations/test_bundle.py

+
+
+def test_package_resource_only_reference():
+    _success_test("tests/data/bundle/package_resource_only_reference.json",


the data files for the tests are quite long. we should use simpler examples (only required fields) for easier debugging when a test fails

jsancio · 2016-10-17T22:51:02Z

@jsancio i though we agreed that we'd have the validation in cosmos, no?

We should have validation in Cosmos, Universe and in the CLI. Let's you, @larioj and I sync offline on this.

Thanks!

larioj · 2016-10-17T23:19:24Z

Note that we changed jsonschema version because we run into this bug: python-jsonschema/jsonschema#201

cruhland · 2016-10-17T23:54:04Z

cli/dcoscli/package/main.py

+    # get the path to the output directory
+    if output_directory is None:
+        package_json_dir = os.path.dirname(package_json_path)
+        output_directory = os.path.join(package_json_dir, "target/")


Nitpick: the trailing slash in target/ shouldn't be needed.

cruhland · 2016-10-18T00:09:11Z

cli/dcoscli/package/main.py

+
+        # create the directory if it does not exist
+        if not (os.path.exists(output_directory)):
+            os.makedirs(output_directory)


Instead of checking for existence, just use the exist_ok parameter:

# ensure the directory exists os.makedirs(output_directory, exist_ok=True)

This avoids a race condition in the current code: if the directory is created by another process after the call to exists() returns, then makedirs() will raise an exception.

Added a test that will check for failure when references are formatted incorrectly

File paths have \ rather than /

jsancio

This looks great @larioj. Some minor comments. 🎉

jsancio · 2016-10-19T00:09:31Z

cli/dcoscli/data/help/package.txt

        Path to a JSON file that contains customized package installation options.
+    --output-directory=<output-directory>
+        Path to the directory where the data should be stored.
+        Defaults to target/ underneath the directory of the DC/OS Package Build Definition.


@larioj What do you think of "... the directory of the file" instead of "... the directory of the DC/OS ..."?

target is very scala-y. dcos node diagnostics defaults to cwd if none specified. i would do the same here.

@tamarrow I would too, and I did. But I got some feedback at the standup that it would be better in target/ so I changed it.

@jsancio How about 'the directory of '?

@tamarrow Can you please talk to @BenWhitehead regarding the target directory and let us know what you guys decide?

@larioj Sounds good 👍

:( I'll still make my vote against it. It's not great that we are inconsistent with where we store files outputted by the CLI.

jsancio · 2016-10-19T00:11:00Z

cli/dcoscli/data/schemas/package-schema.json

@@ -0,0 +1,472 @@
+{


Let's name this file "metadata-schema.json"

jsancio · 2016-10-19T00:12:35Z

cli/dcoscli/data/schemas/package-schema.json

+          "description": "[Deprecated v3.x] An array of strings representing of the requirements file to use for installing the subcommand for Pip. Each item is interpreted as a line in the requirements file."
+        }
+      }
+    },


Actually, @tamarrow what do you think about get rid of this? This means that we can never generate DC/OS Package that are have a Python CLI.

jsancio · 2016-10-19T00:14:17Z

cli/dcoscli/package/main.py

+    build_definition_resolved = build_definition_raw
+
+    # validate resolved build definition
+    package_schema_path = "data/schemas/package-schema.json"


Let's use metadata-schema.json instead. Would need to change all of the related variable names.

jsancio · 2016-10-19T00:16:17Z

cli/dcoscli/package/main.py

+            manifest = json.dumps(manifest_json, indent=2).encode()
+            zip_file.writestr("manifest.json", manifest)
+
+    try:


Can you explain why we are doing this outside of the ContextManager for temp_file?

@jsancio In windows, I can't open the file twice. I tried passing in the opened file to the hash function, but that was giving me incorrect hashes.

@larioj and I talked offline. He is working on a solution.

tamarrow-zz · 2016-10-19T00:28:58Z

@jsancio we can't get rid of that until spark cli is a binary :/

tamarrow-zz · 2016-10-19T00:34:12Z

cli/dcoscli/package/main.py

+    :type build_schema: dict
+    """
+    ref = "marathon"
+    tp = "v2AppMustacheTemplate"


what is tp?

@tamarrow tp stands for template, as in v2AppMustancheTemplate. It's just there so that I don't have to type long names.

What is it is { 'marathon': { 'v2AppMustacheTemplate': 'euhantoeuhsnaoh' }}

lets use template.

tamarrow-zz · 2016-10-19T00:34:55Z

cli/dcoscli/package/main.py

+
+        errs = util.validate_json(build_definition, build_schema)
+        if errs:
+            raise DCOSException('Error validating package: '


this message is used many times in this file. can we have a custom bundle error similarly to how we have a custom marathon error?

tamarrow-zz · 2016-10-19T00:38:36Z

@jsancio when we merge we should mention the jsonschema version change so we can put it in the release notes :)

* updated jsonschema to version 2.5.1

larioj assigned jsancio Oct 13, 2016

jsancio reviewed Oct 13, 2016

View reviewed changes

larioj added 9 commits October 14, 2016 14:07

Add bundle command

fcaaf45

Add resources to binary

0ad55c9

Add resources to binary spec

d6f2639

Address PR comments

f6e9ce3

Fix flake8 errors

5a7fb86

Pass in bundle schema to resolve references

ab52ca6

Make test more readable

7c62720

Error

572cddb

Fix jsonschema error by upgrading to newer version

98d462b

cruhland closed this Oct 15, 2016

jsancio reviewed Oct 16, 2016

View reviewed changes

tamarrow-zz reviewed Oct 17, 2016

View reviewed changes

cli/tests/data/dcos.toml Outdated

@@ -1,5 +1,5 @@

[core]

timeout = 5

Copy link

Contributor

tamarrow-zz Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be reverted

tamarrow-zz reviewed Oct 17, 2016

View reviewed changes

cruhland reviewed Oct 17, 2016

View reviewed changes

cruhland reviewed Oct 18, 2016

View reviewed changes

Jesus E. Larios Murillo added 8 commits October 18, 2016 07:58

Shortened test examples and other PR changes

016fa9e

Add manifest and tests

172fa1e

Added a test that will check for failure when references are formatted incorrectly

Add pydoc to format_cli_version

5e0dbeb

Fix import error

3308707

Add return to function

8a380d4

Fix windows failures

3418f8a

Fix windows error 2

c69d74f

File paths have \ rather than /

Rename bundle>build and package.json>build definition

f670532

jsancio approved these changes Oct 19, 2016

View reviewed changes

tamarrow-zz reviewed Oct 19, 2016

View reviewed changes

Jesus E. Larios Murillo added 3 commits October 19, 2016 17:36

Further PR changes

44eb7da

Change default output-directory to cwd

3116df6

Rename test_build.py to test_package_bulid.py

d369483

larioj changed the title ~~Add bundle command~~ Add package build command Oct 21, 2016

larioj changed the title ~~Add package build command~~ Implement package build command Oct 21, 2016

tamarrow-zz merged commit 1219a29 into dcos:new-install Oct 21, 2016

larioj pushed a commit that referenced this pull request Dec 15, 2016

Implement package build command (#808)

e0816ee

* updated jsonschema to version 2.5.1



		def _resolve_local_references(package_json, package_directory):
		""" Resolves all local refereces in package json

		' specified schema'.format(location))


		def _replace_marathon(bundle_schema, package_directory, package_json):



		def test_package_resource_only_reference():
		_success_test("tests/data/bundle/package_resource_only_reference.json",

Implement package build command #808

Implement package build command #808

Uh oh!

Conversation

larioj commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larioj commented Oct 13, 2016

Uh oh!

jsancio commented Oct 13, 2016

Uh oh!

jsancio commented Oct 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larioj commented Oct 13, 2016 •

edited

Loading