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
Upgrade Ruby to 3.2 #3216
Upgrade Ruby to 3.2 #3216
Conversation
Are you working on this or it is possible to contribute? |
b5b707c
to
47a709c
Compare
@dmitrytrager There are many things to do at the same time to get Huginn working with Ruby 3.2, so I've been doing the work bit by bit. It's not just about our code base but problems lie in obsolete third-party libraries and we need to upgrade or just ditch them if no updates are available.
|
87ef0d5
to
c7da88a
Compare
The ruby-growl gem no longer works and Growl is obsolete.
- Drop poltergeist (PhantomJS) - Drop capybara-screenshot to upgrade capybara - Upgrade webmock and vcr - Enable puma for testing
- Use the `<select>` tag as demanded in version 4 - Turn on `tags` to allow for manual input - Drop complete API response caching and simplify: - Use the AjaxAdapter when cache_response is false - Use the ArrayAdapter when cache_response is true after calling the API
I started to run this on my instance to see if it works fine. |
That is great! Can please you move 930da10 to a separate branch/PR? It makes reviewing the other changes very difficult. |
@dsander Thank you for taking a look! Let me explain. The reformatting is done by rubocop and it introduces ruby 3.2 specific new syntax that cannot be parsed with ruby 2.7. On the other hand, upgrading ruby requires library updates, so the commit itself is just incomplete in the CI point of view, meaning it can't be solely extracted as a working change that passes required tests. So, would you please put up with a commit by commit review? 🥺 |
* foreman needs a bump to be compatible with ruby 3.2 * install supervisor-stdout from git to be compatible with python 3 * adjust configuration of build-in mysql server to newer verrsion
@knu Understood, at first glance the commit looked like a simple I worked a bit on the multi process docker image, it now boots as a fresh container, migrating the mysql database from the old version to the current one still fails though. I also do not know if the required foreman version bump has negative implications for the manual installation guide (would love to get rid of that one 😉 ). |
This looks really annoying, apparently the current image doesn't properly show down the build in mysql server:
As a result of this the new mysql refuses to update the database:
|
Got it working with this commit but I hope we come up with a better solution than installing a binary that was build for 18.04. |
@@ -28,7 +28,7 @@ class CommanderAgent < Agent | |||
|
|||
- If you want to update a WeatherAgent based on a UserLocationAgent, you could use `'action': 'configure'` and set 'configure_options' to `{ 'location': '{{_location_.latlng}}' }`. | |||
|
|||
- In templating, you can use the variable `target` to refer to each target agent, which has the following attributes: #{AgentDrop.instance_methods(false).map { |m| "`#{m}`" }.to_sentence}. | |||
- In templating, you can use the variable `target` to refer to each target agent, which has the following attributes: #{Agent::Drop.instance_methods(false).map { |m| "`#{m}`" }.to_sentence}. |
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.
I am not sure if any Agent gems are using AgentDrop
in a similar way, do you think it is possible/makes sense to also define the new class as AgentDrop
? If not we should probably mention it in the changelog.
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.
GitHub code search didn't find any occurrence of it, but I agree it is worth a mention.
Co-authored-by: Dominik Sander <git@dsander.de>
That hurts, but it's okay as long as it works. 🥺 |
Yeah I think it is better to find a solution that makes the update easier for users that upgraded to a fixed image that properly shuts down the mysql server. A brief look into why supervisord kills the processes but didn't show anything obvious (it seems it never forwards the TERM to the child processes). Have you ever worked with s6-overlay? It looks like a simpler and more modern(?) alternative to supervisord. I have a custom build running on my instance without issues, I am good with the PR. Do you want me to push my mysql5 hack to the branch or do you cherry pick it? |
Like I said before, this looks good to me except that we need a way to support existing users of the build in mysql server. |
This certainly isn't pretty but allows users of the build in mysql server to update. Switching to mysql 8 directly isn't an option at the moment because the docker image never cleanly shuts down it's processes and mysql_upgrade does not work in that case.
@dsander Ah, I just forgot to incorporate your patch. I've cherry-picked the last commit from https://github.com/dsander/huginn/tree/ruby-3.2-mysql-5-hack. How does this look now? |
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.
I have the latest revision running for a few days on my system without issues and the docker specs also pass.
Thanks for all the work on this! 💯
Ruby 2.7 will soon reach EOL. We need to migrate to the latest version of Ruby.
Upgrade Rails to 6.1 #3217
Rails 6.1 is the first version of Rails to support Ruby 3, so we need to do this first.