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

simplify the upgrade so we're backwards compatible #234

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

rebrowning
Copy link
Contributor

see if we can just do this minimal change like we were able to do in st2chatops

@rebrowning rebrowning mentioned this pull request Jan 12, 2024
@nzlosh nzlosh merged commit c1b6d92 into StackStorm:master Jan 13, 2024
5 checks passed
@nzlosh
Copy link
Contributor

nzlosh commented Jan 13, 2024

I've merged the MR, it looks good to me, however I just wonder why you need the other coffeescript pinning when tests were passing for nodejs 20 on master? Is there a unit test that needs to be added to pickup on the error you encountered?

@rebrowning
Copy link
Contributor Author

@nzlosh at this point I'd say consistency and to prevent any future upgrades breaking this unexpectedly (unless we intentionally upgrade coffeescript as well). st2chatops required the pin for coffeescript because of the name change that occurred (coffee-script -> coffeescript). It causes a lot of unexpected failures if an upgrade is attempted because any libraries that changed to the coffeescript name as a dependency are no longer pinned.

@rebrowning rebrowning deleted the update_node_20_min branch January 13, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants