Remove the daemonize feature #378

olleolleolle · 2023-10-07T14:21:54Z

Description:

This is a Pull Request which removes the --daemonize option, its documentation, and all related mentions.

Puma no longer supports it. There are new, other ways to do that.

This closes #359 with a more-final change.

Perhaps this means that there we may need new ways of enabling a Puma::CommonLogger, of enabling non-stdout output (that is "setup_logger") in the Gemstash server.

Now, gemstash operates "in front, in the terminal it is started".

I will abide by the code of conduct.

simi · 2023-10-07T19:31:45Z

Looks good to me, any plan on release? Minor/major bump needed?

olleolleolle · 2023-10-07T20:34:02Z

@simi I could do a full 3.0 major bump for this, it's fine. One could argue for a minor bump since the de-facto drop of this feature was done when we began relying on a Puma 6 that had dropped it (I think in the Puma 6 major --daemonize was removed).

(https://github.com/kigster/puma-daemon#41-using-config-file has good descriptions of what a daemonize solution could look like, now.)

ch1ago · 2023-10-08T16:35:42Z

Awesome! 💯

olleolleolle · 2023-10-08T17:28:24Z

lib/gemstash/cli/start.rb

        @cli.say("Starting gemstash!", :green)
        Puma::CLI.new(args, Gemstash::Logging::StreamLogger.puma_events).run
      end

    private

      def setup_logging
-        return unless daemonize?
-
+        # Enables logging at all times, perhaps there needs to be a new option for this?


This code comment is about "Does the CLI need an option like --run-in-foreground to not redirect the log output into a file?"

Seems like this uses "logger in a signal handler"

Compare this comment, and see how it refers to a nice closure trick from Sidekiq:
resque/resque#1493 (comment)

By removing code which depended on the daemonize?, I got the expected working behaviour.

Fixes #359. Puma 6 no longer supports the --daemonize option. It is not something we wish to support inside Gemstash.

kyrofa · 2023-10-10T16:49:30Z

This broke my prod.

@simi I could do a full 3.0 major bump for this, it's fine. One could argue for a minor bump since the de-facto drop of this feature was done when we began relying on a Puma 6 that had dropped it (I think in the Puma 6 major --daemonize was removed).

Feature aside, I was using the --no-daemonize option. With the removal of that option, gemstash crashed when I upgraded it. Please consider making the CLI args public API, and incorporate it into your versioning scheme. This could have been done in a backward-compatible manner if desired, e.g. by emitting a warning about this option no longer doing anything instead of removing it entirely.

(edit: I originally made a typo and said I was using --daemonize, my apologies)

simi · 2023-10-10T17:44:23Z

@kyrofa that feature was removed in Puma at version 6 and got broken at 14eac03 in gemstash. In last version we just removed that already broken option. Sorry to hear this broke your environment.

kyrofa · 2023-10-10T17:46:54Z

@simi my understanding is not that --no-daemonize broke, but that it became the default behavior, correct? That changed nothing for me: I didn't even notice. But removal of the CLI option did indeed cause breakage.

I think we can agree that 4eac03937e83db6db6365ffdc1fd86b990b8ebf probably should have been a major version bump due to the behavior change. 20/20 hindsight, as they say. But are we really using that arguable mistake to justify putting more breakage in the same major release?

simi · 2023-10-10T17:49:47Z

@simi my understanding is not that --no-daemonize broke, but that it became the default behavior, correct? That changed nothing for me: I didn't even notice. But removal of the CLI option did indeed cause breakage.

Ahh, clear. a66162c#diff-1b2a427dbe98191af04a96ffbfe65d744487281f99dfc8bd6a21de087cb9de72L78-L79 I think we have missed by removing --daemonize also --no-daemonize was removed and that could potentially cause troubles.

Do you think it is worth it to release new version adding back this option back in deprecated manner?

kyrofa · 2023-10-10T17:55:49Z

Do you think it is worth it to release new version adding back this option back in deprecated manner?

I think that would probably be appreciated by other folks using this option. It gives them a chance to understand why these things were removed and update their usage without breaking them. I understand it's more work, of course, and to be clear I've fixed this on my end. Think about it!

simi · 2023-10-10T17:58:37Z

Do you think it is worth it to release new version adding back this option back in deprecated manner?

I think that would probably be appreciated by other folks using this option. It gives them a chance to understand why these things were removed and update their usage without breaking them.

Clear, let us create the update.

The idea behind this was --daemonize doesn't work anymore and by removing it we don't break any setup (since it doesn't work anyway). But since --no-daemonize is automatically created by thor cli DSL, we have missed that is going to be removed as well as currently valid option causing no troubles and that could cause these issues. 😞

kyrofa · 2023-10-10T17:59:02Z

Ohhhhhhh I totally missed that the inverse option was created automatically. That makes WAY more sense. That said, turning --daemonize from "I'm not behaving the way you want" to crashing is still a second round of breakage (where the first round was initially making --daemonize not behave properly). My two cents.

simi · 2023-10-10T18:07:05Z

Ohhhhhhh I totally missed that the inverse option was created automatically. That makes WAY more sense. That said, turning --daemonize from "I'm not behaving the way you want" to crashing is still a second round of breakage (where the first round was initially making --daemonize not behave properly). My two cents.

--daemonize wasn't "I'm not behaving the way you want", but crashing already in 2.6.x

[retro@retro  ps5]❤ bundle exec gemstash start --daemonize
Starting gemstash!
bundler: failed to load command: gemstash (/home/retro/.gem/ruby/3.2.2/bin/gemstash)
/home/retro/.gem/ruby/3.2.2/gems/puma-6.4.0/lib/puma/cli.rb:45:in `initialize': invalid option: --daemon (OptionParser::InvalidOption)
        from /home/retro/.gem/ruby/3.2.2/gems/gemstash-2.6.0/lib/gemstash/cli/start.rb:16:in `new'
        from /home/retro/.gem/ruby/3.2.2/gems/gemstash-2.6.0/lib/gemstash/cli/start.rb:16:in `run'
        from /home/retro/.gem/ruby/3.2.2/gems/gemstash-2.6.0/lib/gemstash/cli.rb:83:in `start'
        from /home/retro/.gem/ruby/3.2.2/gems/thor-1.2.2/lib/thor/command.rb:27:in `run'
        from /home/retro/.gem/ruby/3.2.2/gems/thor-1.2.2/lib/thor/invocation.rb:127:in `invoke_command'
        from /home/retro/.gem/ruby/3.2.2/gems/thor-1.2.2/lib/thor.rb:392:in `dispatch'
        from /home/retro/.gem/ruby/3.2.2/gems/thor-1.2.2/lib/thor/base.rb:485:in `start'
        from /home/retro/.gem/ruby/3.2.2/gems/gemstash-2.6.0/lib/gemstash/cli.rb:34:in `start'
        from /home/retro/.gem/ruby/3.2.2/gems/gemstash-2.6.0/exe/gemstash:6:in `<top (required)>'
        from /home/retro/.gem/ruby/3.2.2/bin/gemstash:25:in `load'
        from /home/retro/.gem/ruby/3.2.2/bin/gemstash:25:in `<top (required)>'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/cli/exec.rb:58:in `load'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/cli/exec.rb:58:in `kernel_load'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/cli/exec.rb:23:in `run'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/cli.rb:492:in `exec'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/cli.rb:34:in `dispatch'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/cli.rb:28:in `start'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.5.0.dev/exe/bundle:37:in `block in <top (required)>'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/site_ruby/3.2.0/bundler/friendly_errors.rb:117:in `with_friendly_errors'
        from /home/retro/.rubies/ruby-3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.5.0.dev/exe/bundle:29:in `<top (required)>'
        from /home/retro/.gem/ruby/3.2.2/bin/bundle:25:in `load'
        from /home/retro/.gem/ruby/3.2.2/bin/bundle:25:in `<main>'

kyrofa · 2023-10-10T18:08:33Z

Ah, then I am mistaken indeed, and I understand where you're coming from now. Thank you for your patience!

simi · 2023-10-10T18:09:22Z

Ah, then I am mistaken indeed, and I understand where you're coming from now. Thank you for your patience!

Thanks for you patience as well, let's release the 2.7.1 soon adding back no-op --no-daemonize with warning.

kyrofa · 2023-10-10T18:09:53Z

Thank you @simi 🤗

simi · 2023-10-12T11:30:34Z

@kyrofa 0f8e2ab 💪

ch1ago mentioned this pull request Oct 8, 2023

CLI daemonized cannot be set to true. fixes #359 #376

Closed

olleolleolle commented Oct 8, 2023

View reviewed changes

olleolleolle marked this pull request as ready for review October 8, 2023 17:30

olleolleolle force-pushed the cli_daemonized_false branch 2 times, most recently from 07474ff to e938bdc Compare October 8, 2023 17:40

Remove the daemonize feature

fc0e06b

Fixes #359. Puma 6 no longer supports the --daemonize option. It is not something we wish to support inside Gemstash.

olleolleolle force-pushed the cli_daemonized_false branch from e938bdc to fc0e06b Compare October 8, 2023 17:56

olleolleolle merged commit a66162c into main Oct 8, 2023
12 checks passed

olleolleolle deleted the cli_daemonized_false branch October 8, 2023 18:05

This was referenced Oct 11, 2023

Release a 2.7.1 supporting a --no-daemonize option that only outputs a warning #380

Closed

Reinstate nonfunctional --no-daemonize, --daemonize options that warn #381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the daemonize feature #378

Remove the daemonize feature #378

olleolleolle commented Oct 7, 2023 •

edited

Loading

simi commented Oct 7, 2023

olleolleolle commented Oct 7, 2023

ch1ago commented Oct 8, 2023

olleolleolle Oct 8, 2023

olleolleolle Oct 8, 2023

kyrofa commented Oct 10, 2023 •

edited

Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023 •

edited

Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023 •

edited

Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023 •

edited

Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023

simi commented Oct 12, 2023

Remove the daemonize feature #378

Remove the daemonize feature #378

Conversation

olleolleolle commented Oct 7, 2023 • edited Loading

Description:

simi commented Oct 7, 2023

olleolleolle commented Oct 7, 2023

ch1ago commented Oct 8, 2023

olleolleolle Oct 8, 2023

Choose a reason for hiding this comment

olleolleolle Oct 8, 2023

Choose a reason for hiding this comment

kyrofa commented Oct 10, 2023 • edited Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023 • edited Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023 • edited Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023 • edited Loading

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023

simi commented Oct 10, 2023

kyrofa commented Oct 10, 2023

simi commented Oct 12, 2023

olleolleolle commented Oct 7, 2023 •

edited

Loading

kyrofa commented Oct 10, 2023 •

edited

Loading

kyrofa commented Oct 10, 2023 •

edited

Loading

kyrofa commented Oct 10, 2023 •

edited

Loading

kyrofa commented Oct 10, 2023 •

edited

Loading