-
Notifications
You must be signed in to change notification settings - Fork 602
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
Disable autostart for non 'server' Rails commands #2239
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously booting a Rails app via launching the Rails console or running a Rake task would prevent the agent from autostarting, but other Rails commands such as `rails routes` would still see the agent autostart. Now the `:'autostart.denylisted_constants'` config option's default list of constants has been updated to include all of the `Rails::Command` based command classes that either boot a Rails app or otherwise don't hide via `hide_command!`. This existing denylisted constants based approach was chosen given what little we have to work with when taking alternative approaches. When a Rails user runs `bin/rails routes` in their app directory, the `railties/lib/rails/commands.rb` file ends up `shift`-ing the `'routes'` value out of `ARGV` and `ARGV` is empty (`[]`) when it reaches the agent. With `ARGV` not providing us anything to hook into, we have to either rely on walking the callstack to determine a caller or checking for constants to be defined. Given the precedent for checking for constants to ignore Rails console contexts, that approach was built upon to ignore the other Rails CLI/TUI contexts.
Rails commands being ignored, PR 2239
NOTE that these additional commands were not skipped over previously due to an oversight or anything. The agent simply predates their existence. The additional commands either did not exist at all previously or were handled differently as Rake tasks, and the agent already ignores some* Rake tasks by default. |
tannalynn
reviewed
Sep 29, 2023
The agent already has conventions in place for using a separate 'test' environment specific configuration for test running, and for denying specific Rake tasks by name, so do not redundantly disable them by default with the constants denylist. This way, users who wish to use the agent with their tests and Rake tasks can continue to do so.
Updated CHANGELOG to reflect c6dae08 Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
tannalynn
reviewed
Sep 29, 2023
tannalynn
reviewed
Sep 29, 2023
Rails command autostart denylisting - better explain that this impacts Rails 7 users, as Rails 5-6 users who ran something like `rails routes` would have seen the "routes" subcommand handed off to Rake and be handled by the agent's Rake related denylist functionality. Thanks, @tannalynn! Co-authored-by: Tanna McClure <[email protected]>
CHANGELOG entry for newly ignored Rails commands, thanks @kaylareopelle Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
SimpleCov Report
|
kaylareopelle
approved these changes
Sep 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously booting a Rails app via launching the Rails console or running a Rake task would prevent the agent from autostarting, but other Rails commands such as
rails routes
would still see the agent autostart.Now the
:'autostart.denylisted_constants'
config option's default list of constants has been updated to include all of theRails::Command
based command classes that either boot a Rails app or otherwise don't hide viahide_command!
.This existing denylisted constants based approach was chosen given what little we have to work with when taking alternative approaches. When a Rails user runs
bin/rails routes
in their app directory, therailties/lib/rails/commands.rb
file ends upshift
-ing the'routes'
value out ofARGV
andARGV
is empty ([]
) when it reaches the agent. WithARGV
not providing us anything to hook into, we have to either rely on walking the callstack to determine a caller or checking for constants to be defined. Given the precedent for checking for constants to ignore Rails console contexts, that approach was built upon to ignore the other Rails CLI/TUI contexts.resolves #2086