Change the pack schema #2988

emedvedev · 2016-11-01T20:44:19Z

As discussed with @Kami, we need to do something about packs meant for newer StackStorm versions than whatever the user might have. Ideally, StackStorm should be able to figure it out and ask the user to update if they want to install a newer pack:

$ st2 pack install sensu

ERROR: You are running StackStorm 2.1dev, but this pack requires StackStorm 3.14. 

Consider updating or install an older version of the pack: sensu=940faea3 is 
the latest version that would work for you (requires 1.6).

This PR does not implement this feature, but it lays the groundwork by preventing StackStorm from throwing on unknown pack.yaml fields. We also add properties that are very likely to appear in the next version: dependencies and system. Even if there's no implementation for these fields right now, they are pretty self-explanatory (and there's a description in the schema, too) and helpful to the user, so it's better if 2.1 still allows to install a pack meant for whenever those features are implemented. Then users can handle versions and deps themselves or consider updating.

dependencies will be implemented either in the content management release (see https://github.com/stackstorm/discussions/issues/204), or earlier, but in a basic form: for instance, conflicting dependencies won't be resolved.

system is a way to specify system requirements. We could use it to require a minimal StackStorm version, but also for packages. Even if StackStorm can't check and install system requirements right now, at least with this field users would know where to look for them. I'll try to take a stab at it it as soon as I can, but only after everything critical is done for the current release.

codecov-io · 2016-11-01T20:52:35Z

Current coverage is 76.67% (diff: 89.80%)

Merging #2988 into master will increase coverage by 0.18%

@@             master      #2988   diff @@
==========================================
  Files           426        427     +1   
  Lines         21812      21959   +147   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          16684      16836   +152   
+ Misses         5128       5123     -5   
  Partials          0          0

Diff Coverage	File Path
••••••• 75%	new st2common/st2common/util/versioning.py
•••••••••• 100%	st2common/st2common/models/db/pack.py
•••••••••• 100%	st2common/st2common/models/api/pack.py
•••••••••• 100%	st2common/st2common/bootstrap/base.py
•••••••••• 100%	st2common/st2common/constants/pack.py

Powered by Codecov. Last update 9329a60...2412e7e

emedvedev · 2016-11-01T21:32:12Z

Discussed this schema change with @bigmstone, I think we came up with a decent structure for storing system requirements:

name: mypack
description: A pack that does things
[...]
system:
  stackstorm: 2.1
  packages:
    - libxml
    - pkg2
    - pkg3
  deb-packages:
    - libpcap-dev
    - happy-package
  rpm-packages:
    - libpcap-devel
    - happy_package

In this PR, we'll just introduce the field, and in the future versions st2 will act upon the params inside system that it knows about: e.g. if stackstorm is specified, it would check if the version matches. If handling of some parameters isn't implemented in a particular StackStorm version, it's still pretty self-explanatory for a user (falling back to good old manual labor).

lakshmi-kannan · 2016-11-01T22:24:56Z

st2tests/st2tests/fixtures/packs/dummy_pack_9/pack.yaml

+dependencies :
+  - core=0.2.0
+system :
+  stackstorm : 1.6dev


Looks like we’ll need a range and not a simple number for this. So when we actually start validating these fields, we should make sure that we don't lock ourselves on specifying an exact version.

I also think that st2_version should be a top level field and system should just be used for OS level package deps. @Kami what are your thoughts?

Looks like we’ll need a range and not a simple number for this. So when we actually start validating these fields, we should make sure that we don't lock ourselves on specifying an exact version.

The number was supposed to be a minimum version, but you're right, we could find ourselves deprecating or removing stuff, so I'm adding full range support.

emedvedev · 2016-11-01T23:36:20Z

I also think that st2_version should be a top level field and system should just be used for OS level package deps. @Kami what are your thoughts?

Yeah, could use @Kami's opinion on this. My thinking is system would collect not just packages, but various other system-level dependencies: additional packages, StackStorm version, required action runners if we decide to pack them separately, Ruby version if the pack implements a Ruby runner, etc. I can get behind making StackStorm version special, so I'm not exactly opposed, but I'd still rather put all external requirements in one place to have consistency. Otherwise we'll just find ourselves wondering if we should promote every new kind of requirement. Is the ruby version special for a ruby runner pack? Is it "top level" special or just sorta special? Etc.

So I'm fine with either, but I think system for requirements and system.stackstorm for st2 version is better because there's consistency and even some degree of tidiness, so to speak. There's not enough consistency and tidiness in the package management world as it is, let's not contribute to that mess and set a good example of being neat 😛

Kami · 2016-11-02T08:15:39Z

st2common/st2common/bootstrap/base.py

@@ -177,11 +177,6 @@ def _register_pack_db(self, pack_name, pack_dir):
        pack_file_list = get_file_list(directory=pack_dir, exclude_patterns=EXCLUDE_FILE_PATTERNS)
        content['files'] = pack_file_list

-        # Note: If some version values are not explicitly surrounded by quotes they are recognized
-        # as numbers so we cast them to string
-        if 'version' in content:


Any particular reason why this was removed?

Comment mentioned why it was needed :D Or it's not needed anymore now that we use semver everywhere?

Shouldn't be needed anymore. Can get it back just in case :)

Yeah, you can actually just leave it as is - I will make this change in the versioning change and move it to a better place anyway (API model) :)

Kami · 2016-11-02T08:16:47Z

st2common/st2common/models/api/pack.py

                'items': {'type': 'string'},
                'default': []
+            },
+            'system': {


Yeah - I think this name is much better than engines. We could perhaps also call it system_version or st2_version to be more explicit and obvious what is going on.

I would also simplify it and make it a string, I think there is little need to have an object since we have only one platform - StackStorm.

In Node.JS land it's a bit different since there are multiple executions environments using different engines / JITs. Having said that, I also very rarely seen people specify anything else than nodejs there :)

I would also simplify it and make it a string, I think there is little need to have an object since we have only one platform - StackStorm.

Yeah, if we stuck to the initial "engines" purpose, that would be "st2_version", but later (#2988 (comment) and #2988 (comment)) there was an idea to utilize it for storing system-level requirements like packages and versions of runners, etc.

Kami · 2016-11-02T08:20:31Z

st2common/st2common/models/api/pack.py

-
-        model = cls.model(name=name, description=description, ref=ref, keywords=keywords,
-                          version=version, author=author, email=email, files=files)
+        parameters = pack.__dict__


I'm personally not a fan of this __dict__ approach, too much magic and could cause us to store weird stuff.

I would prefer to explicitly assign it as we did before. As far as forward compatibility goes - I thought we will just accept them without throwing, but not actually store them in the database (which I think it's the right thing to do).

Actually, now that I think about it - new version of mongoengine throws if you pass in a field which is not explicitly declared so this would actually cause it to throw :)

In any case, as mentioned above, I would prefer to explicitly assign parameters we use and store and ignore others.

I thought we will just accept them without throwing, but not actually store them in the database (which I think it's the right thing to do).

Reasonable. Will do.

Kami · 2016-11-02T08:23:27Z

st2common/tests/integration/test_register_content_script.py

@@ -144,5 +144,5 @@ def test_register_setup_virtualenvs(self):
        # All packs
        cmd = BASE_CMD_ARGS + ['--register-setup-virtualenvs', '--register-no-fail-on-failure']
        exit_code, stdout, stderr = run_command(cmd=cmd)
-        self.assertTrue('Setup virtualenv for 8 pack(s)' in stderr)
+        self.assertTrue('Setup virtualenv for 10 pack(s)' in stderr)


Yeah, this is a bit error prone, for got to update this in my PR so we don't need to update it every time new pack fixtures are added (I did do this in one place, but forgot some others :)).

Will push a commit to this branch which changes that.

Haha. I thought it's written like this on purpose, to manually check the pack number or something. 😛

Kami · 2016-11-02T08:29:16Z

I also think that st2_version should be a top level field and system should just be used for OS level package deps. @Kami what are your thoughts?

Yeah, I agree :)

As mentioned in my comment above, I would also prefer st2_version to be a simple string since there is not much need for anything else.

Kami · 2016-11-02T08:30:49Z

I added some comments - some things which still need to be decided and changes, but overall nice work 👍

I also just checked out this branch and will make the version changes we discussed yesterday directly in this branch so there won't be any merge conflicts :)

specifiers (e.g. 1.0). In case we encounter a non-semver valid version, we cast it so semver valid one by appending .0 at the end of the original version string.

This way the schema is still forward compatible, but we don't try to store or pass in the fields we don't officially support. This way user can't pass garbage or bad data and cause us to store it.

… fixture is added.

…enly normalized everywhere and so it's also done before .validate() is called.

…ow valid semver format.

…ex semver version specifiers.

…parts.

…on strings.

arm4b · 2016-11-02T14:54:03Z

@emedvedev @bigmstone
RE: #2988 (comment)

  deb-packages:
    - libpcap-dev
    - happy-package
  rpm-packages:
    - libpcap-devel
    - happy_package

Even if that part is not implemented, just a thought: I think it will be a bit dangerous to specify/install OS-level dependencies, because it could be much harder than we initially think.

Different packages can provide same libraries(package1|package2), different flavors of distros can have different package naming, dependency could be installed/configured from sources and not from the package, add here OS package versioning requirements and so on underwater rocks.

pip doesn't install OS-level dependencies, why should we? Maybe it could be a bit of a nightmare, I don't know. But it's good to think about the change like that carefully.

… pack and compare it against the current StackStorm version. If the version required is not satisfied, throw and fail pack registration.

Various pack schema changes

Kami · 2016-11-02T18:08:27Z

@emedvedev I merged my PR into this one so only thing we need to decide is version normalization (cc @m4dcoder, @lakshmi-kannan).

Besides that, we should be good to go.

Oh, and of course we also need some docs + upgrade notes on it (I can tackle upgrade notes once it's merged).

Kami · 2016-11-02T20:55:51Z

@armab

Even if that part is not implemented, just a thought: I think it will be a bit dangerous to specify/install OS-level dependencies, because it could be much harder than we initially think.

I definitely think StackStorm should not be be touching or handling OS-level dependencies. This info is more just like a metadata for packaging (and perhaps package maintainers can use it to auto-generate files based on this metadata).

emedvedev · 2016-11-02T20:57:23Z

Yeah, what that guy said. :)

arm4b · 2016-11-03T10:26:00Z

@emedvedev @Kami ^^ Huh, I thought you smoked something, but looks like I've smoked something wrong 😄

Just let's make sure the naming is not confusing, so smokers like me won't think that os-level packages will be really installed. Kind of recommends keyword.

emedvedev · 2016-11-04T05:59:05Z

@Kami I've removed the .0 stuff as I think it should be a separate discussion in another PR, and the version validation should probably be done during the installation for a few reasons, so also removed the check (but not utils). So normalization can be returned later, I just didn't want to hold off this PR until everyone has time for discussions.
Merging.

emedvedev added 4 commits November 1, 2016 20:46

Extend pack schema to support required versions and dependencies

5c70569

Lazy pack population

151fc87

Add test cases for new pack meta and future values

b73f1cd

Use pack_9 and pack_10 fixtures

40f41bd

emedvedev force-pushed the pack-schema branch from be5bc37 to 40f41bd Compare November 1, 2016 20:47

Change "engines" to "system"

5aeb6cb

Relax the system schema

bbb0562

lakshmi-kannan reviewed Nov 1, 2016

View reviewed changes

Version ranges for stackstorm checks

433f6a8

emedvedev added 2 commits November 2, 2016 06:48

Improve the version range check

b2c0722

Update tests for version range

50cf673

Kami reviewed Nov 2, 2016

View reviewed changes

Kami added 6 commits November 2, 2016 10:03

Update pack validation so we still support old, non-semver valid version

a2228a6

specifiers (e.g. 1.0). In case we encounter a non-semver valid version, we cast it so semver valid one by appending .0 at the end of the original version string.

Explicitly assign and pass fields to the models.

13fa933

This way the schema is still forward compatible, but we don't try to store or pass in the fields we don't officially support. This way user can't pass garbage or bad data and cause us to store it.

Make "stackstorm_version" field top-level on the Pack model.

4d659e2

Make some tests more dynamic so they don't break when a new pack test…

a593300

… fixture is added.

Fix some typos.

17019d3

Update affected tests.

8c996e9

Kami mentioned this pull request Nov 2, 2016

Various pack schema changes #2990

Merged

5 tasks

Kami added 3 commits November 2, 2016 11:10

Move version normalization to the PackAPI constructor so it's consist…

428a6be

…enly normalized everywhere and so it's also done before .validate() is called.

Add tests for pack version normalization.

92d1a86

Throw more user-friendly exception on pack version which doesn't foll…

04552bd

…ow valid semver format.

Kami added 9 commits November 2, 2016 11:22

Add test for invalid pack semver version.

ee44c8c

Use double line break.

d29ee1e

Fix lint.

6506ad0

Also put additional check and validation to the db model level.

b057b85

Add custom uility function which knows how to compare and match compl…

9bce03d

…ex semver version specifiers.

Update stackstorm_version specifier so it actually uses valid semver …

ecfbe6b

…parts.

Add test for versioning utils.

201f41c

Update ST2_VERSION_REGEX so it actually requires a valid semver versi…

1b80871

…on strings.

Update more affected tests.

bde037a

Kami added 5 commits November 2, 2016 17:49

Update changelog.

4b7626c

When registering a pack, check stackstorm_version if specified in the…

4b18181

… pack and compare it against the current StackStorm version. If the version required is not satisfied, throw and fail pack registration.

Add tests case for it.

fa5a4b3

Fix lint.

2a673af

Merge pull request #2990 from StackStorm/pack_schema_versioning_changes

713ea5e

Various pack schema changes

emedvedev added 3 commits November 3, 2016 20:28

Remove version normalization and stackstorm_version check

3b43f36

Get string casting back

5c906eb

Remove version checks

2412e7e

emedvedev merged commit 4c88cd5 into master Nov 4, 2016

LindsayHill deleted the pack-schema branch October 3, 2018 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the pack schema #2988

Change the pack schema #2988

emedvedev commented Nov 1, 2016 •

edited

codecov-io commented Nov 1, 2016 •

edited

emedvedev commented Nov 1, 2016 •

edited

lakshmi-kannan Nov 1, 2016

emedvedev Nov 1, 2016

emedvedev commented Nov 1, 2016 •

edited

Kami Nov 2, 2016

emedvedev Nov 2, 2016

Kami Nov 2, 2016

Kami Nov 2, 2016

Kami Nov 2, 2016

emedvedev Nov 2, 2016

Kami Nov 2, 2016 •

edited

Kami Nov 2, 2016

emedvedev Nov 2, 2016

Kami Nov 2, 2016

emedvedev Nov 2, 2016

Kami commented Nov 2, 2016

Kami commented Nov 2, 2016

arm4b commented Nov 2, 2016 •

edited

Kami commented Nov 2, 2016

Kami commented Nov 2, 2016

emedvedev commented Nov 2, 2016

arm4b commented Nov 3, 2016 •

edited

emedvedev commented Nov 4, 2016

Change the pack schema #2988

Change the pack schema #2988

Conversation

emedvedev commented Nov 1, 2016 • edited

codecov-io commented Nov 1, 2016 • edited

Current coverage is 76.67% (diff: 89.80%)

emedvedev commented Nov 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emedvedev commented Nov 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kami Nov 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kami commented Nov 2, 2016

Kami commented Nov 2, 2016

arm4b commented Nov 2, 2016 • edited

Kami commented Nov 2, 2016

Kami commented Nov 2, 2016

emedvedev commented Nov 2, 2016

arm4b commented Nov 3, 2016 • edited

emedvedev commented Nov 4, 2016

emedvedev commented Nov 1, 2016 •

edited

codecov-io commented Nov 1, 2016 •

edited

emedvedev commented Nov 1, 2016 •

edited

emedvedev commented Nov 1, 2016 •

edited

Kami Nov 2, 2016 •

edited

arm4b commented Nov 2, 2016 •

edited

arm4b commented Nov 3, 2016 •

edited