Skip to content

Avoid needing a shell to support Distroless Container Images (in most cases)#299

Merged
ArturT merged 63 commits intomainfrom
avoid-shell
May 21, 2025
Merged

Avoid needing a shell to support Distroless Container Images (in most cases)#299
ArturT merged 63 commits intomainfrom
avoid-shell

Conversation

@ArturT
Copy link
Copy Markdown
Member

@ArturT ArturT commented Apr 17, 2025

Story

https://trello.com/c/NmNCGtoE

Related

Based on PR:

Description

Avoid needing a shell to support Distroless Container Images (in most cases).

If you pass an environment variable to KNAPSACK_PRO_RSPEC_TEST_EXAMPLE_DETECTOR_PREFIX like "MY_CUSTOM_VARIABLE=value bundle exec" then shell is required when running an RSpec dry run task to determine test examples for slow test files. This will not work in Distroless Container Image.

Checklist reminder

  • You added the changes to the UNRELEASED section of the CHANGELOG.md, including the needed bump (ie, patch, minor, major)
  • You follow the architecture outlined below for RSpec in Queue Mode, which is a work in progress (feel free to propose changes):
    • Pure: lib/knapsack_pro/pure/queue/rspec_pure.rb contains pure functions that are unit tested.
    • Extension: lib/knapsack_pro/extensions/rspec_extension.rb encapsulates calls to RSpec internals and is integration and e2e tested.
    • Runner: lib/knapsack_pro/runners/queue/rspec_runner.rb invokes the pure code and the extension to produce side effects, which are integration and e2e tested.

@ArturT ArturT added the wip Work in progress label Apr 17, 2025
Comment thread lib/knapsack_pro/runners/spinach_runner.rb Outdated
@3v0k4 3v0k4 changed the base branch from master to main April 30, 2025 08:14
Comment thread .circleci/config.yml Outdated
@ArturT ArturT removed the wip Work in progress label May 19, 2025
Comment thread lib/knapsack_pro/adapters/rspec_adapter.rb Outdated
Comment thread lib/knapsack_pro/adapters/rspec_adapter.rb Outdated
Copy link
Copy Markdown
Contributor

@3v0k4 3v0k4 left a comment

Choose a reason for hiding this comment

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

Good job!

Comment thread CHANGELOG.md Outdated
Comment thread README.md Outdated
Comment thread distroless/Dockerfile Outdated
Comment thread README.md Outdated
Comment thread lib/knapsack_pro/repository_adapters/git_adapter.rb Outdated
Comment thread lib/knapsack_pro/repository_adapters/git_adapter.rb Outdated
Comment thread lib/knapsack_pro/repository_adapters/git_adapter.rb Outdated
Comment on lines +61 to +79
it 'returns test example paths for slow test files' do
logger = instance_double(Logger)
allow(KnapsackPro).to receive(:logger).and_return(logger)
allow(logger).to receive(:info)

cmd = 'bundle exec rake knapsack_pro:rspec_test_example_detector'
env = { 'RACK_ENV' => 'test', 'RAILS_ENV' => 'test' }
expect(Kernel).to receive(:system).with(env, cmd).and_return(true)

rspec_test_example_detector = instance_double(KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector)
expect(KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector).to receive(:new).and_return(rspec_test_example_detector)

test_file_example_paths = double
expect(rspec_test_example_detector).to receive(:test_file_example_paths).and_return(test_file_example_paths)

expect(subject).to eq test_file_example_paths

cmd = 'RACK_ENV=test RAILS_ENV=test bundle exec rake knapsack_pro:rspec_test_example_detector'
expect(Kernel).to receive(:system).with(cmd).and_return(cmd_result)
expect(logger).to have_received(:info).with("Generating RSpec test examples JSON report for slow test files to prepare it to be split by test examples (by individual test cases). Thanks to that, a single slow test file can be split across parallel CI nodes. Analyzing 5 slow test files.")
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note for the future:

These tests need too many doubles so it's pretty fragile.

If we want to test behavior with so many side effects, let's use an integration test. If we want to test specifics (Kernel receives some arguments), let's create a separate method and unit test just that (without stubbing so many things).

I would have picked the latter in this case.

Comment thread spec/knapsack_pro/adapters/rspec_adapter_spec.rb Outdated
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
@ArturT ArturT merged commit 47c849d into main May 21, 2025
25 checks passed
@ArturT ArturT deleted the avoid-shell branch May 21, 2025 10:26
@ArturT ArturT changed the title Avoid needing a shell to support distroless environment Avoid needing a shell to support distroless environments (in most cases) May 21, 2025
@ArturT ArturT changed the title Avoid needing a shell to support distroless environments (in most cases) Avoid needing a shell to support Distroless Container Images (in most cases) May 21, 2025
@ArturT ArturT mentioned this pull request May 21, 2025
2 tasks
ArturT added a commit that referenced this pull request May 29, 2025
@ArturT ArturT mentioned this pull request May 29, 2025
2 tasks
ArturT added a commit that referenced this pull request May 29, 2025
This PR reverts changes from 8.2.0 version that have been introduced in:

* #299

to address the issue reported in:

* #301

But we keep some code simplification:
* Keep Kernel.exec for Spinach to avoid catching the exit code
* Keep: fd13e84 - we removed the unneeded `EXTRA_TEST_FILES_DELAY` env var in CircleCI yml that was accidentally added some time ago.
namespace :queue do
task :rspec, [:rspec_args] do |_, args|
Kernel.exec("RAILS_ENV=test RACK_ENV=test #{$PROGRAM_NAME} 'knapsack_pro:queue:rspec_go[#{args[:rspec_args]}]'")
Kernel.exec({ 'RAILS_ENV' => 'test', 'RACK_ENV' => 'test' }, $PROGRAM_NAME, "knapsack_pro:queue:rspec_go[#{args[:rspec_args]}]")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@ArturT @3v0k4 we dropped the single quotes in this, which could cause some weirdness

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you are referring to the missing ' in 'knapsack_pro:queue:rspec_go[#{args[:rspec_args]}]', that was left out intentionally.

Previously, the quotes were used to ensure the arguments were parsed correctly. However, since we now pass "knapsack_pro:queue:rspec_go[#{args[:rspec_args]}]" as a positional argument, they are no longer necessary. Argument parsing still works, for example, running: bundle exec rake "knapsack_pro:queue:rspec[--format d --no-color]" will use the documentation format and disable color in the output.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm, yea it should be getting invoked right. I'd love to have repro steps for #301, it's frustrating attempting to figure out what's happening from very little info :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd love to have repro steps for #301, it's frustrating attempting to figure out what's happening from very little info :(

I know the struggle. Reproducing unexpected edge cases without enough information can be really tricky.

@liath liath mentioned this pull request Jun 4, 2025
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.

3 participants