Add support for switching between nodejs versions (upgrade/downgrade) #222
Conversation
cognifloyd
force-pushed
the
upgrade_nodejs
branch
from
cc9c6a3
to
aed896b
Compare
cognifloyd
force-pushed
the
upgrade_nodejs
branch
from
aed896b
to
84d00df
Compare
cognifloyd
force-pushed
the
upgrade_nodejs
branch
6 times, most recently
from
7faa500
to
3a24960
Compare
|
Checks are green. Please review and merge. |
cognifloyd
changed the title
pull-request-size
bot
added
the
size/S
|
Thanks for the PR. What means under the "ensure the nodesource repo is up-to-date before attempting to install nodejs"? Why the nodejs repository needs to be deleted first? Do you mean switching between major nodejs versions maybe? |
|
Yes, this is for upgrading between major nodejs versions. This is part of the And Each major nodejs version uses a different URL. But, both repo versions are installed with a release RPM file that has the same version. The only way to tell which one is installed is to inspect the installed repo file. If you do not adjust the repo when upgrading to a new major nodejs version, yum will not be able to find the appropriate rpms. |
cognifloyd
force-pushed
the
upgrade_nodejs
branch
from
3a24960
to
9a1839e
Compare
|
Rebased, updated the task name, and added some explanatory comments in the tasks file to explain why this is required. |
cognifloyd
changed the title
|
@cognifloyd ignore my banter about the template file, i think keeping it as an RPM is the right approach... think the biggest thing here is making the behavior consistent across RHEL + Debian and it should be good to go! |
The nodesource repo rpm causes problems if a node.js major version upgrade is required (such as from 4.x to 10.x) because the version number of the repo version does not change even though the node.js version in the repo file changes. Rather than hacking around that problematic rpm, we use the yum_repository ansible module to install the repo.
cognifloyd
changed the title
|
Switched to the yum_repository module to drop the hacky repo upgrade tasks. Excellent suggestion. Thanks @nmaludy |
|
@cognifloyd re: NPM repos -> I discovered this approach for the st2-chatops packaging container - I wonder which way we may wish to consider standardizing on. |
|
I'm not sure we can harmonize much between the dockerfiles and config management (at least ansible config management) because the goals of the install methods are different. Ansible strives for idempotence wherever possible. Running scripts that just "do" things is typically anathema to that goal. The script gets things installed, but you cannot safely re-run a playbook that runs such a script. As a script does not report whether or not it changes things, ansible assumes that running scripts always changes things. So you also cannot use a playbook that runs such a script to audit the installation. But, Dockerfiles are glorified scripts that save state after each RUN step. There is no idempotence or granular change tracking. So just running a script is a perfectly sane thing to do to minimize the number of docker layers while keeping the steps as reusable as possible. So, I don't see much benefit to changing the dockerfile here. Some things do warrant standardization across installation methods, so that they each install the same features or versions for example. But, I don't think standardizing beyond "install node.js" is worth the effort in this case. |
|
@cognifloyd - my real point was the dnf command pinning the major version of node. Thoughts about that? Maybe I'm reading the thread too fast and missing something. I was working on this last week and came to the conclusion that `RUN dnf -y module install nodejs:10` got me what I needed more efficiently that other methods. IANANode Expert though.
|
|
@punkrokk Ah. I don't have a CentOS 8 machine handy, but I believe |
|
CentOS8 is a topic for a dedicated PR for which we have a TODO issue: #248 and out of scope of work in this PR. I like how the code ended in this PR, thanks @nmaludy for review! 👍 It also needs manual cycle to test the node repo upgrade behavior end2end to identify any corner cases before merging. |
|
@cognifloyd can you please run this manually to do the upgrade 2-3 times and report back? Thanks! |
|
I think @cognifloyd as contributor already ran his code during development. However in PR both contributor and reviewer are responsible for the merged code. I would prefer if reviewer does additional few checks too if it's not simplest change. Once we'll do a StackStorm release,- besides of good amount of automated tests (testing knowns) we all do extensive manual checking trying random things based on changelog (testing unknowns) for several days. That's usually a source of good amount of bugs caught before landing into production version. Sometimes we test in production too :), but I'd like us to keep the quality bar as @StackStorm/TSC best effort. |
|
Using the Vagrantfile that came with ansible-st2, I changed the default box to CentOS 7: And added some extra_vars to go back to the last st2 version that used nodejs 4: I then destroyed the vagrant box and did |
|
More background on why this is needed. In v0.9.1, node.js defaults to v6. Bringing up the box, everything installed fine. The nodesource repo was installed with the rpm, and the repo file is: Then I removed my ansible extra_vars and reprovision with the same older version of the playbook. nodejs does not update (it stays at version 6 as that is the default in that v0.9.1 of this repo): And as expected installing st2chatops fails because nodejs>=8.0 is not installed: Then I checkout master (again dropping my ansible extra_vars, but still targeting centos7), run The nodesource repo does not update and installing Then, I switched to the upgrade_nodejs branch (this PR), rsynced the repo, and re-ran the provisioner. Though the repo seems to update, it fails to use the newer repo because the repo installed by the rpm is loaded first. So we also need to remove the RPM: Here's the repo file list: I will add a task to remove the rpm and then I'll re-run. |
cognifloyd
force-pushed
the
upgrade_nodejs
branch
from
02338cd
to
465778e
Compare
cognifloyd
force-pushed
the
upgrade_nodejs
branch
from
8380d06
to
a8181c1
Compare
|
K. That removed the repo rpm, but the cache was out-of-date, so it did not find the nodejs rpm. So, we need a register on both repo tasks to make sure if either of them changes, the cache gets updated when installing nodejs. |
|
Then I ran This time the nodesource repo and nodejs upgraded as expected: |
|
I ran some tests with the Vagrant file included with ansible-st2 on this branch and everything seems good to go! Added the following to the Vagrantfile to force an old nodejs version: Then did the following |
arm4b
changed the title
|
Thanks for PR, review and testing! 👍 |
|
@armab you changed the title to say this supports upgrade/downgrade, but really it only supports upgrade. There's a commented out line that we need to enable on the yum: nodejs install task if we want to allow downgrading. |
Add some tasks to ensure the nodesource repo is up-to-date before attempting to install nodejs.
Thus, if this playbook is run against a machine that already has the repo, it will safely upgrade
the repo so that the nodejs install task doesn't complain about not finding the version requested.
This also sets update_cache on upgrade so that the newer nodejs rpm becomes available for install.