-
-
Notifications
You must be signed in to change notification settings - Fork 777
Pack dependency management #4769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
49e1c8f to
e1afd79
Compare
e1afd79 to
139709c
Compare
Fixes https://github.com/StackStorm/discussions/issues/354 1. `packs.install` workflow changed from `achtion-chain` to `orquesta` for workflow complexity. 2. Download all packs and dependency packs, check for conflict packs before `install pack requirements` and `register pack`. 3. Workflow fails on any time if conflict pack is recognized, downloaded packs will be removed. 4. Register packs in reverse order, it means the last dependency pack will be installed first. 5. New option `--skip` (default value is False) for skipping dependencies check: `st2 install pack --skip` 6. Only case that will check conflict is that if pack was installed and dependency pack has specified pack version. 7. Nested level of dependencies to prevent infinite or long download loops is 3. 8. Support StackStorm Exchange packs, the private repo ULR and local file system for conflict pack checking.
139709c to
d47e267
Compare
Two new test files for testing get_pack_dependencies.py and virtualenv_setup_prerun.py New test cases for download dependencies packs. New test cases for `skip` install dependency flag. Modified install workflow for more simplicity.
m4dcoder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got some questions.
When install/remove list of packs, if the pack `ref` is not same as pack `name`, the pack information is not printed. See file: https://github.com/StackStorm/st2/blob/master/st2tests/st2tests/fixtures/packs/dummy_pack_19/pack.yaml For example: `st2 pack install files:///dummy_pack_19 csv` Actually result: +---------------+-------------------------------+---------+--------------+ | name | description | version | author | +---------------+-------------------------------+---------+--------------+ | email | E-Mail Actions/Sensors for | 1.1.3 | James Fryman | | | StackStorm | | | +---------------+-------------------------------+---------+--------------+ Expected result: +-------------------+---------------+-------------------------------+---------+--------------+ | ref | name | description | version | author | +-------------------+---------------+-------------------------------+---------+--------------+ | dummy_pack_19_ref | dummy_pack_19 | dummy pack | 0.1.0 | st2-dev | | email | email | E-Mail Actions/Sensors for | 1.1.3 | James Fryman | | | | StackStorm | | | +-------------------+---------------+-------------------------------+---------+--------------+ Root Cause: Pack install/remove is not query `ref` as print out parameter, and only pack `name` is checked to find out pack. When pack `name` and `ref` is not the same, pack information is not printed.
7216010 to
6bb6512
Compare
|
Also, how are you doing e2e tests? The unit tests here are not sufficient. |
I am working on it. Have't found good way to do it. Suggestions are welcomed. |
|
@jinpingh You can try to do that as integration tests like |
@m4dcoder
|
|
@jinpingh You have to spend some more thoughts on the list of test cases. At the minimum, you did not include what I asked for above with handling conflict against a pack that is already installed on the system. Also, if we support x level of nested dependencies then testing 1 level is not sufficient. |
From my above list Did manually tested 4 level (supported level is 3) nested dependencies and last level of dependencies are not installed. I can go with same with I am still working on Thanks! |
|
Please separate 3 into different test cases and be precise. There could be different types of conflicts. |
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising!
I'll start running this branch and trying new pack install with dependencies next week to see how it behaves and provide more detailed feedback.
Talking about code. There are a lot of questionable assumptions made and code duplication. Instead of duplicating logic and implementation between pack components we should be reusing common functions and make sure logic is shared between all the pack actions. This will ensure we don't end in situation when pack download accepts one format while pack dependency follows another.
However it's a great starting prototype for a quick demo phase so everyone can try and provide early feedback 👍
|
@armab @m4dcoder This and StackStorm/st2tests#179 are ready for review. Manually tested with packages build by CircleCI and StackStorm/st2tests#179 on Ubuntu 16.04. |
|
@blag Awesome! I'll battle-test the feature locally. |
|
First thing, looks like in order to use new |
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packs metadata https://github.com/StackStorm/st2/blob/fb2932166408d2572dae9ee57c8e64c212b2d1a3/contrib/packs/pack.yaml needs a major version increment per new functionality/changes made in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second run of st2 pack install https://github.com/StackStorm/stackstorm-ms fails, after first successful run. Looks like the workflow mistakenly assumes dependency conflict for the 2nd run.
This will result in broken re-installations of the same packs for users. Needs fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failure in processing dependencies should output helpful error message to the user, where was the problem and how to act to resolve the issue:
$ st2 pack install https://github.com/StackStorm/stackstorm-ms
[ succeeded ] init_task
[ succeeded ] download_pack
[ succeeded ] make_a_prerun
[ succeeded ] get_pack_dependencies
[ succeeded ] check_dependency_and_conflict_list
[ succeeded ] stop_installation_and_cleanup_because_conflict
id: 5d84cec86cb8de08ab157533
action.ref: packs.install
parameters:
packs:
- https://github.com/StackStorm/stackstorm-ms
python3: false
status: failed
start_timestamp: Fri, 20 Sep 2019 13:06:16 UTC
end_timestamp: Fri, 20 Sep 2019 13:06:21 UTC
result:
errors:
- message: Execution failed. See result for details.
task_id: fail
type: error
output:
packs_list:
- microsoft_test
+--------------------------+------------------------+-----------------------------------+-----------------------------+-------------------------------+
| id | status | task | action | start_timestamp |
+--------------------------+------------------------+-----------------------------------+-----------------------------+-------------------------------+
| 5d84cec86cb8de033487d866 | succeeded (0s elapsed) | init_task | core.noop | Fri, 20 Sep 2019 13:06:16 UTC |
| 5d84cec96cb8de033487d869 | succeeded (1s elapsed) | download_pack | packs.download | Fri, 20 Sep 2019 13:06:17 UTC |
| 5d84cecb6cb8de033487d86c | succeeded (0s elapsed) | make_a_prerun | packs.virtualenv_prerun | Fri, 20 Sep 2019 13:06:19 UTC |
| 5d84cecb6cb8de033487d86f | succeeded (1s elapsed) | get_pack_dependencies | packs.get_pack_dependencies | Fri, 20 Sep 2019 13:06:19 UTC |
| 5d84cecc6cb8de033487d872 | succeeded (0s elapsed) | check_dependency_and_conflict_lis | core.noop | Fri, 20 Sep 2019 13:06:20 UTC |
| | | t | | |
| 5d84cecd6cb8de033487d875 | succeeded (0s elapsed) | stop_installation_and_cleanup_bec | packs.delete | Fri, 20 Sep 2019 13:06:21 UTC |
| | | ause_conflict | | |
+--------------------------+------------------------+-----------------------------------+-----------------------------+-------------------------------+
As a user, I have no idea what's wrong behind the:
- message: Execution failed. See result for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--skip-dependencies param makes no difference and tries to check and install pack dependencies anyway
Update: Confirming --skip-dependencies works well 👍 as API restart was required per code changes.
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides of the above issues, looks like workflow is running and first install with dependencies is happening, so we're close.
Please also prepare the respective st2docs documentation change for this new and highly requested functionality.
24e35cd to
105c588
Compare
| ``url_hosts_blacklist`` and ``url_hosts_whitelist`` runner attribute. (new feature) | ||
| #4757 | ||
| * Add ``user`` parameter to ``re_run`` method of st2client. #4785 | ||
| * Install pack dependencies automatically. #4769 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog message could be improved for this new functionality.
|
|
||
| # Try one more time to get existing pack version by name if 'stackstorm-' is in | ||
| # pack name | ||
| if not existing_pack_version and 'stackstorm-' in pack_name.lower(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad assumption thinking that stackstorm- is in pack name and take actions based on that.
OK about this for now, could be improved in future.
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m4dcoder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for your diligence in completing and closing this feature.
Fixes https://github.com/StackStorm/discussions/issues/354
Resolves #3336
Closes #3698
packs.installworkflow changed fromachtion-chaintoorquestafor workflow complexity.install pack requirementsandregister pack.--skip(default value is False) for skipping dependencies check:st2 install pack --skip