Skip to content
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

Add python warning if install a pack that only supports python 2 #5037

Merged
merged 12 commits into from
Sep 16, 2020

Conversation

amanda11
Copy link
Contributor

@amanda11 amanda11 commented Sep 8, 2020

packs.download action will log a warning if attempt to install a pack that only supports python 2

packs.install workflow will include an extra step to check if the metadata indicates warnings (and will return a warning_list of any of the packs that had a warning - whereby warnings will only be raised if the pack supports python 2). The warning_list will also be an output of the workflow (similar to conflict_list)

st2 client, when running "pack install" will output to screen any warnings reported in the warning_list from the workflow execution.

Part of #4938

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Sep 8, 2020
@amanda11 amanda11 marked this pull request as draft September 8, 2020 15:43
@amanda11
Copy link
Contributor Author

amanda11 commented Sep 8, 2020

I wanted to get some feedback before I started adding UT and integration tests.

I did consider changing the download_pack so the warning was returned rather than just Success, but was concerned that might cause integration problems if the result of that action were checked. So therefore decided to just log a warning and keep the warning as an extra result to the workflow (similar to conflicts)

Here is some output from runs. In this scenario test1 pack is python 2 &3, test2 pack is python2, test 3 pack is dependant on pack 2 & 3.

Output when use CLI to install test3 pack:

st2 pack install file:///home/centos/test3 --force

	[ succeeded ] init_task
	[ succeeded ] download_pack
	[ succeeded ] make_a_prerun
	[ succeeded ] get_pack_dependencies
	[ succeeded ] check_dependency_and_conflict_list
	[ succeeded ] download_pack
	[ succeeded ] make_a_prerun
	[ succeeded ] get_pack_dependencies
	[ succeeded ] check_dependency_and_conflict_list
	[ succeeded ] install_pack_requirements
	[ succeeded ] get_pack_warnings
	[ succeeded ] register_pack

+-------+-------+------------------+---------+------------------+
| ref   | name  | description      | version | author           |
+-------+-------+------------------+---------+------------------+
| test1 | Test1 | Pack for both    | 0.0.1   | Ammeon Solutions |
|       |       | pythons          |         |                  |
| test2 | Test2 | Pack for both    | 0.0.1   | Ammeon Solutions |
|       |       | pythons          |         |                  |
| test3 | Test3 | Pack for both    | 0.0.1   | Ammeon Solutions |
|       |       | pythons          |         |                  |
+-------+-------+------------------+---------+------------------+
DEPRECATION WARNING: Pack Test2 only supports Python 2.x. ST2 will remove support for Python 2.x in a future release.

Output from execution get on the above workflow:

st2 execution get 5f57622006b2c171a620addb
id: 5f57622006b2c171a620addb
action.ref: packs.install
parameters: 
  force: true
  packs:
  - file:///home/centos/test3
  python3: false
status: succeeded (60s elapsed)
start_timestamp: Tue, 08 Sep 2020 10:51:12 UTC
end_timestamp: Tue, 08 Sep 2020 10:52:12 UTC
result: 
  output:
    conflict_list: []
    message: Successfully installed packs
    packs_list:
    - test3
    - test1
    - test2
    warning_list:
    - 'DEPRECATION WARNING: Pack Test2 only supports Python 2.x. ST2 will remove support for Python 2.x in a future release.'
+--------------------------+-------------------------+------------+------------+-----------------+
| id                       | status                  | task       | action     | start_timestamp |
+--------------------------+-------------------------+------------+------------+-----------------+
| 5f576221ef7e4b4ff9e813f8 | succeeded (1s elapsed)  | init_task  | core.noop  | Tue, 08 Sep     |
|                          |                         |            |            | 2020 10:51:13   |
|                          |                         |            |            | UTC             |
| 5f576222ef7e4b4ff9e81408 | succeeded (3s elapsed)  | download_p | packs.down | Tue, 08 Sep     |
|                          |                         | ack        | load       | 2020 10:51:14   |
|                          |                         |            |            | UTC             |
| 5f576225ef7e4b4ff9e81418 | succeeded (1s elapsed)  | make_a_pre | packs.virt | Tue, 08 Sep     |
|                          |                         | run        | ualenv_pre | 2020 10:51:17   |
|                          |                         |            | run        | UTC             |
| 5f576227ef7e4b4ff9e81428 | succeeded (2s elapsed)  | get_pack_d | packs.get_ | Tue, 08 Sep     |
|                          |                         | ependencie | pack_depen | 2020 10:51:19   |
|                          |                         | s          | dencies    | UTC             |
| 5f576229ef7e4b4ff9e81438 | succeeded (1s elapsed)  | check_depe | core.noop  | Tue, 08 Sep     |
|                          |                         | ndency_and |            | 2020 10:51:21   |
|                          |                         | _conflict_ |            | UTC             |
|                          |                         | list       |            |                 |
| 5f57622aef7e4b4ff9e81448 | succeeded (2s elapsed)  | download_p | packs.down | Tue, 08 Sep     |
|                          |                         | ack        | load       | 2020 10:51:22   |
|                          |                         |            |            | UTC             |
| 5f57622def7e4b4ff9e81458 | succeeded (1s elapsed)  | make_a_pre | packs.virt | Tue, 08 Sep     |
|                          |                         | run        | ualenv_pre | 2020 10:51:25   |
|                          |                         |            | run        | UTC             |
| 5f57622fef7e4b4ff9e81468 | succeeded (1s elapsed)  | get_pack_d | packs.get_ | Tue, 08 Sep     |
|                          |                         | ependencie | pack_depen | 2020 10:51:27   |
|                          |                         | s          | dencies    | UTC             |
| 5f576231ef7e4b4ff9e81478 | succeeded (0s elapsed)  | check_depe | core.noop  | Tue, 08 Sep     |
|                          |                         | ndency_and |            | 2020 10:51:29   |
|                          |                         | _conflict_ |            | UTC             |
|                          |                         | list       |            |                 |
| 5f576232ef7e4b4ff9e81488 | succeeded (35s elapsed) | install_pa | packs.setu | Tue, 08 Sep     |
|                          |                         | ck_require | p_virtuale | 2020 10:51:30   |
|                          |                         | ments      | nv         | UTC             |
| 5f576256ef7e4b4ff9e81498 | succeeded (2s elapsed)  | get_pack_w | packs.get_ | Tue, 08 Sep     |
|                          |                         | arnings    | pack_warni | 2020 10:52:06   |
|                          |                         |            | ngs        | UTC             |
| 5f576259ef7e4b4ff9e814a8 | succeeded (2s elapsed)  | register_p | packs.load | Tue, 08 Sep     |
|                          |                         | ack        |            | 2020 10:52:09   |
|                          |                         |            |            | UTC             |
+--------------------------+-------------------------+------------+------------+-----------------+

Output from execution get on the packs.download action:

st2 execution get 5f57622aef7e4b4ff9e81448
id: 5f57622aef7e4b4ff9e81448
action.ref: packs.download
context.user: st2admin
parameters: 
  dependency_list:
  - file:///home/centos/test1
  - file:///home/centos/test2
  force: true
  packs:
  - file:///home/centos/test3
  python3: false
status: succeeded (2s elapsed)
start_timestamp: Tue, 08 Sep 2020 10:51:22 UTC
end_timestamp: Tue, 08 Sep 2020 10:51:24 UTC
result: 
  exit_code: 0
  result:
    test1: Success.
    test2: Success.
  stderr: 'st2.actions.python.DownloadGitRepoAction: DEBUG    Force mode is enabled, deleting lock file...
    st2.actions.python.DownloadGitRepoAction: DEBUG    Detected local pack directory which is not a git repository, just copying files over...
    st2.actions.python.DownloadGitRepoAction: DEBUG    Moving pack from /root/597177fe4c61b2c9eabc3b2c23217374 to /opt/stackstorm/packs/.
    st2.actions.python.DownloadGitRepoAction: DEBUG    Force mode is enabled, deleting lock file...
    st2.actions.python.DownloadGitRepoAction: DEBUG    Detected local pack directory which is not a git repository, just copying files over...
    st2.actions.python.DownloadGitRepoAction: DEBUG    Moving pack from /root/e1475b30771849db813af564d67d4e47 to /opt/stackstorm/packs/.
    st2.actions.python.DownloadGitRepoAction: WARNING  DEPRECATION WARNING: Pack test2 only supports Python 2.x. ST2 will remove support for Python 2.x in a future release.
    '
  stdout: ''

@amanda11
Copy link
Contributor Author

amanda11 commented Sep 8, 2020

Screenshot from UI showing how it looks on the pack.install run:
image

Screenshot from UI showing the download pack warning from above run:
image

@amanda11
Copy link
Contributor Author

amanda11 commented Sep 8, 2020

Example of output from CLI when install 2 packs that are only python 2:

st2 pack install file:///home/centos/packs/test2 file:///home/centos/packs/test4

	[ succeeded ] init_task
	[ succeeded ] download_pack
	[ succeeded ] make_a_prerun
	[ succeeded ] get_pack_dependencies
	[ succeeded ] check_dependency_and_conflict_list
	[ succeeded ] install_pack_requirements
	[ succeeded ] get_pack_warnings
	[ succeeded ] register_pack

+-------+-------+------------------+---------+------------------+
| ref   | name  | description      | version | author           |
+-------+-------+------------------+---------+------------------+
| test2 | Test2 | Pack for both    | 0.0.1   | Ammeon Solutions |
|       |       | pythons          |         |                  |
| test4 | Test4 | Pack for both    | 0.0.1   | Ammeon Solutions |
|       |       | pythons          |         |                  |
+-------+-------+------------------+---------+------------------+
DEPRECATION WARNING: Pack Test2 only supports Python 2.x. ST2 will remove support for Python 2.x in a future release.
DEPRECATION WARNING: Pack Test4 only supports Python 2.x. ST2 will remove support for Python 2.x in a future release.

And the execution result:

st2 execution get 5f57a999551860ca3cbac6fc
id: 5f57a999551860ca3cbac6fc
action.ref: packs.install
parameters: 
  packs:
  - file:///home/centos/packs/test2
  - file:///home/centos/packs/test4
  python3: false
status: succeeded (23s elapsed)
start_timestamp: Tue, 08 Sep 2020 15:56:09 UTC
end_timestamp: Tue, 08 Sep 2020 15:56:32 UTC
result: 
  output:
    conflict_list: []
    message: Successfully installed packs
    packs_list:
    - test2
    - test4
    warning_list:
    - 'DEPRECATION WARNING: Pack Test2 only supports Python 2.x. ST2 will remove support for Python 2.x in a future release.'
    - 'DEPRECATION WARNING: Pack Test4 only supports Python 2.x. ST2 will remove support for Python 2.x in a future release.'
+--------------------------+------------------------+------------+------------+-----------------+
| id                       | status                 | task       | action     | start_timestamp |
+--------------------------+------------------------+------------+------------+-----------------+
| 5f57a99a4a206e0f9ede5f9f | succeeded (1s elapsed) | init_task  | core.noop  | Tue, 08 Sep     |
|                          |                        |            |            | 2020 15:56:10   |
|                          |                        |            |            | UTC             |
| 5f57a99b4a206e0f9ede5faf | succeeded (2s elapsed) | download_p | packs.down | Tue, 08 Sep     |
|                          |                        | ack        | load       | 2020 15:56:11   |
|                          |                        |            |            | UTC             |
| 5f57a99e4a206e0f9ede5fbf | succeeded (1s elapsed) | make_a_pre | packs.virt | Tue, 08 Sep     |
|                          |                        | run        | ualenv_pre | 2020 15:56:14   |
|                          |                        |            | run        | UTC             |
| 5f57a99f4a206e0f9ede5fcf | succeeded (2s elapsed) | get_pack_d | packs.get_ | Tue, 08 Sep     |
|                          |                        | ependencie | pack_depen | 2020 15:56:15   |
|                          |                        | s          | dencies    | UTC             |
| 5f57a9a24a206e0f9ede5fdf | succeeded (0s elapsed) | check_depe | core.noop  | Tue, 08 Sep     |
|                          |                        | ndency_and |            | 2020 15:56:18   |
|                          |                        | _conflict_ |            | UTC             |
|                          |                        | list       |            |                 |
| 5f57a9a34a206e0f9ede5fef | succeeded (6s elapsed) | install_pa | packs.setu | Tue, 08 Sep     |
|                          |                        | ck_require | p_virtuale | 2020 15:56:19   |
|                          |                        | ments      | nv         | UTC             |
| 5f57a9aa4a206e0f9ede5fff | succeeded (1s elapsed) | get_pack_w | packs.get_ | Tue, 08 Sep     |
|                          |                        | arnings    | pack_warni | 2020 15:56:26   |
|                          |                        |            | ngs        | UTC             |
| 5f57a9ac4a206e0f9ede600f | succeeded (3s elapsed) | register_p | packs.load | Tue, 08 Sep     |
|                          |                        | ack        |            | 2020 15:56:28   |
|                          |                        |            |            | UTC             |
+--------------------------+------------------------+------------+------------+-----------------+

Comment on lines 46 to 47
PACK_PYTHON2_WARNING = "DEPRECATION WARNING: Pack %s only supports Python 2.x. " \
"ST2 will remove support for Python 2.x in a future release."
Copy link
Member

Choose a reason for hiding this comment

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

Small note about "future release(s)" and also would be nice to add a call to action for the users:

Suggested change
PACK_PYTHON2_WARNING = "DEPRECATION WARNING: Pack %s only supports Python 2.x. " \
"ST2 will remove support for Python 2.x in a future release."
PACK_PYTHON2_WARNING = "DEPRECATION WARNING: Pack %s only supports Python 2.x. " \
"Python 2 support will be dropped in the future releases. " \
"Please consider updating your packs to work with Python 3."

Feel free to adjust as you wish.

@arm4b
Copy link
Member

arm4b commented Sep 8, 2020

Thanks for providing the CLI examples! Everything feels right 👍

@arm4b arm4b added this to the 3.3.0 milestone Sep 8, 2020
@arm4b arm4b added this to In Progress in Python 2 Deprecation via automation Sep 8, 2020
@amanda11 amanda11 marked this pull request as ready for review September 8, 2020 21:22
CHANGELOG.rst Outdated Show resolved Hide resolved
@arm4b arm4b self-requested a review September 10, 2020 19:32
@arm4b arm4b requested review from a team September 11, 2020 17:30
@arm4b
Copy link
Member

arm4b commented Sep 11, 2020

@StackStorm/maintainers @StackStorm/contributors Anyone wants to try this new deprecation warning experience and provide the feedback? Help wanted to try this feature out.

@arm4b arm4b requested a review from winem September 11, 2020 17:44
@Sheshagiri
Copy link
Contributor

@armab I will give this a try and post the experience/feedback here.

@winem
Copy link
Contributor

winem commented Sep 13, 2020

Hi, I tried to test this but I must be doing something wrong or miss something. First I patched one of my dev environments with your changes and then I tried it on a plain new CentOS8 VM by running curl -sSL https://stackstorm.com/packages/install.sh | bash -s -- --user=st2admin --password='xxx' --dev=st2/11676 to use the artifacts build by circleci.

st2 --version
st2 3.3dev (3f3326ef7), on Python 3.6.8

For some reason the warning_list after running pack.install is always empty. I tried it with nagios, telegram and https://github.com/StackStorm-Exchange/stackstorm-test2 which is used by the unit tests as well. I also verified that all your changes are part of the installed packages.

Any idea what I'm doing wrong or any better way to test this?

@amanda11
Copy link
Contributor Author

Are those packs python 2 only? The test2 one you referred to isn't. The pack.yaml doesn't specify python versions - so it assumes it's both.
It only reports the warning if the pack's pack.yank states python 2 only.

@amanda11
Copy link
Contributor Author

Nagios as well isn't a python 2 only pack according to it's pack.yaml
I did notice discrepancies between what is shown on the exchange main page and what is in it's pack.yaml in some cases.
Eg some say python 2 only but their supported versions list both.

@winem
Copy link
Contributor

winem commented Sep 13, 2020

Thanks @amanda11, that was exactly the issue. I just trusted the info shown on the stackstorm exchange. Everything works as expected with my own packs and forks of a pack with a modified pack.yaml.

I'll run some more tests via the UI and CLI and will let you know if I find anything. So far everything matches exactly what you described above and I like the user experience.

@amanda11
Copy link
Contributor Author

@armab @nmaludy on the stackstorm exchange overview page where does the supported python versions come from? It doesn't seem to match the pack.yaml of files in exchange

@Sheshagiri
Copy link
Contributor

@amanda11 from what I know that info is served from https://github.com/StackStorm-Exchange/index/tree/master/v1/packs which is updated whenever a pr is merged to master in https://github.com/StackStorm-Exchange. If there are discrepancies then it could be that in some instances(just thinking loud here) the ci for index wouldn't have run or had issues.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks @amanda11 for the well organized PR and @winem @Sheshagiri for the review!
Looks good to me 👍

@arm4b
Copy link
Member

arm4b commented Sep 15, 2020

As an addition to this, besides of pack install CLI warning, there might be also a good idea to log similar Deprecation message for st2actionrunner during the pack install execution.

@amanda11
Copy link
Contributor Author

amanda11 commented Sep 15, 2020

@armab Warnings that we get at moment in the actionrunner.log are:

  1. WARNING when the pack install does a download
    st2.actions.python.DownloadGitRepoAction: WARNING DEPRECATION WARNING: Pack Test2 only supports Python 2.x. Python 2 support will be dropped in future releases. Please consider updating your packs to work with Python 3.x
  2. The warning_list comes out in the audit:
    2020-09-15 22:19:13,791 139907786138800 AUDIT base [-] Liveaction completed (liveaction_db={'status': 'succeeded', 'runner_info': {u'hostname': u'centos-7-devsource', u'pid': 12107}, 'workflow_execution': u'5f613dd12235164e0be18521', 'parameters': {u'packs_status': {u'test2': u'Success.'}}, 'action_is_workflow': False, 'start_timestamp': '2020-09-15 22:19:12.149032+00:00', 'delay': None, 'callback': {}, 'task_execution': u'5f613de0a287f1ad7e3ffe31', 'notify': None, 'result': {'stdout': u'', 'exit_code': 0, 'stderr': '', 'result': {u'warning_list': [u'DEPRECATION WARNING: Pack Test2 only supports Python 2.x. Python 2 support will be dropped in future releases. Please consider updating your packs to work with Python 3.x']}}, 'context': {u'orquesta': {u'workflow_execution_id': u'5f613dd12235164e0be18521', u'task_id': u'get_pack_warnings', u'task_execution_id': u'5f613de0a287f1ad7e3ffe31', u'task_name': u'get_pack_warnings', u'task_route': 1}, u'user': u'stanley', u'parent': {u'user': u'stanley', u'execution_id': u'5f613dd022bf8267a2bebe1b', u'pack': u'packs'}, u'pack': u'packs'}, 'action': u'packs.get_pack_warnings', 'id': '5f613de0a287f1ad7e3ffe33', 'end_timestamp': '2020-09-15 22:19:13.626455+00:00'})

Does this cover the logging needed in actionrunner, or is there anywhere else its needed?

@arm4b
Copy link
Member

arm4b commented Sep 16, 2020

@amanda11 That's awesome! Yes, that logging example is more than enough 👍
Thanks for checking.

@amanda11
Copy link
Contributor Author

No worries - I thought I had a log in there somewhere, but forgot to put in the examples.
I will merge.

@amanda11 amanda11 merged commit f17e0b0 into master Sep 16, 2020
Python 2 Deprecation automation moved this from In Progress to Done Sep 16, 2020
@amanda11 amanda11 deleted the add_python_warning_pack_install branch September 16, 2020 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants