Skip to content

Conversation

@FooBarWidget
Copy link
Member

@FooBarWidget FooBarWidget commented Apr 10, 2025

Upgrades daemon_controller to 3.0.2.

Closes #1970.

@FooBarWidget FooBarWidget requested review from CamJN and Copilot April 10, 2025 08:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • CHANGELOG: Language not supported

Copy link
Member

@CamJN CamJN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a few cases I commented on a code style thing in one place but I mean the style applies generally.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 8 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@CamJN CamJN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last comment, maybe we should have passenger set its exiting defaults at the call sites for daemon controller, so that we don't mess things up for our customers again?

# See doc/OPTIONS.md for options docs.
def initialize(identifier:, start_command:, ping_command:, pid_file:, log_file:,
lock_file: nil, stop_command: nil, restart_command: nil, before_start: nil,
start_timeout: 30, start_abort_timeout: 10, stop_timeout: 30,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're changing the defaults here, and @tinco said that caused issues for at least one customer the last time we did it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that you flag this. The changing of defaults is necessary to solve some issues, including issues that we've run into. I think it should be acceptable if we communicate this properly. Maybe bump the minor version again? That is, we go to 6.2.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure can you bump the version and update the changelog with the changed defaults then?

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.

Error during shutdown on Heroku with 5.1.7

3 participants