search

StackStorm / ansible-st2

Ansible Roles and Playbooks to deploy StackStorm
https://galaxy.ansible.com/StackStorm/stackstorm/
Apache License 2.0
100 stars 77 forks source link

Add support for nodejs 14 and list the variable on the README #299

Closed winem closed 3 years ago

winem commented 3 years ago

There were different ways to realize the node 14 support but this one is the only one that's not breaking.

Changing just the default would break deployments of older releases even though we may mention it on the README. 2nd challenge was that a simple comparison with the ansible version test is not able to work with latest as st2_version. That's why we use an if-elsif-else and not just an if-else clause here.

winem commented 3 years ago

Looks good, couple of comments.

Does this cope if you specify st2_version as 3.5.0-1 - as for Ubuntu we have to specify the full version of package, whereas for El we can just use 3.5.0.

Also probably best to compare with st2chatops _version instead of st2_version.

Yes, it does. I tested it with 3.5.0-1, 3.4.1-2 and latest.

I'll mark the PR as draft for now and do another round of tests with the st2chatops_version later tonight.

Is there a reason why st2chatops_version does not reference st2_version as default?

amanda11 commented 3 years ago

Looks good, couple of comments. Does this cope if you specify st2_version as 3.5.0-1 - as for Ubuntu we have to specify the full version of package, whereas for El we can just use 3.5.0. Also probably best to compare with st2chatops _version instead of st2_version.

Yes, it does. I tested it with 3.5.0-1, 3.4.1-2 and latest.

I'll mark the PR as draft for now and do another round of tests with the st2chatops_version later tonight.

Is there a reason why st2chatops_version does not reference st2_version as default?

st2chatops_version and st2_version aren't necessarily the same on ubuntu. e.g. for 3.4.1 - then st2_version needs to be 3.4.1-2 and st2chatops_version needs to be 3.4.1-1. (If it was just EL then everything woulsd be nice and simple and just one variable.... - I also think the roles are generally designed to be independant, but if you find other cases where the st2chatops role picks up st2_version, then I don't have an issue with st2chatops_version defaulting to st2_version. Though you do need to make sure you check the package versions, so if we end up defaulting them I can see people not bothering to check, and then accidentally installing 3.4.1-1 st2 - which isn't the latest...

winem commented 3 years ago

I wasn't aware that both are released independent from each other. So that's what I call a good reason to have the vars being independent as well. Thanks!

Tests went fine so this PR is open for final reviews.