Allow building a stemcell with a custom agent (jammy)#508
Allow building a stemcell with a custom agent (jammy)#508mkocher wants to merge 1 commit intoubuntu-jammyfrom
Conversation
📝 WalkthroughWalkthroughAdds optional support for injecting a prebuilt bosh-agent into development stemcell builds via the BOSH_AGENT_BIN_PATH environment variable; updates README, passes the variable through build environment generation, and conditionally uses the provided binary in the build script. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.86.0)bosh-stemcell/lib/bosh/stemcell/build_environment.rbError: The Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 93-94: Remove the trailing space at the end of the sentence "While
building a stemcell for development purposes it is possible to set the
BOSH_AGENT_BIN_PATH environment variable to inject an agent binary into the
stemcell instead of downloading one." in README.md; edit the line (no code
change otherwise) so there is no trailing whitespace character at the end of
that paragraph.
In `@stemcell_builder/stages/bosh_go_agent/apply.sh`:
- Around line 43-49: Quote the variable expansions to prevent word-splitting:
when reading the bosh_agent_version use bosh_agent_version="$(cat
"${assets_dir}/bosh-agent-version")" (quote ${assets_dir}), and update the meta4
invocation to quote the path and generated filename like
--metalink="${assets_dir}/metalink.meta4" and
--file="bosh-agent-${bosh_agent_version}-linux-amd64"; keep existing quoting for
"${BOSH_AGENT_BIN_PATH}" in the cp call. This ensures assets_dir and
bosh_agent_version are safely expanded in apply.sh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43212182-52bf-41fb-88ea-cfb479d52bd0
📒 Files selected for processing (3)
README.mdbosh-stemcell/lib/bosh/stemcell/build_environment.rbstemcell_builder/stages/bosh_go_agent/apply.sh
Set BOSH_AGENT_BIN_PATH=./bosh-agent to provide a bosh agent binary to be injected into the stemcell being built. Useful when doing agent development work.
mkocher
force-pushed
the
custom-agent-jammy
branch
from
391d3c5 to
d59c2ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bosh-stemcell/lib/bosh/stemcell/build_environment.rb`:
- Around line 238-242: passthrough_settings_from_environment currently returns
environment keys with their original casing which can emit lowercase keys (e.g.,
bosh_agent_bin_path) and break downstream checks; update
passthrough_settings_from_environment so it emits keys normalized to uppercase
(e.g., BOSH_AGENT_BIN_PATH) while retaining their values: when selecting keys
from environment (using the existing keep list and environment.select),
transform the selected key names to upcase before returning so apply.sh and
other consumers will see the expected uppercase variable names.
- Around line 103-106: Add unit specs for the new passthrough merge path to
cover BOSH_AGENT_BIN_PATH presence, absence and casing for the command_env
behavior: in the existing describe for `#command_env` add contexts that set the
subject's env to include 'BOSH_AGENT_BIN_PATH' (e.g. '/tmp/bosh-agent') and
assert command_env includes "BOSH_AGENT_BIN_PATH='/tmp/bosh-agent'"; another
context with env = {} asserting command_env does not include
'BOSH_AGENT_BIN_PATH='; and an additional context that verifies behavior for
alternate casing (e.g. 'bosh_agent_bin_path' or 'Bosh_Agent_Bin_Path') to
document whether passthrough_settings_from_environment/hash_as_bash_env treats
keys case-sensitively. Ensure these specs exercise command_env,
passthrough_settings_from_environment and hash_as_bash_env so regressions are
caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: deb8ad65-5064-4558-9dac-b1496e5ed96c
📒 Files selected for processing (3)
README.mdbosh-stemcell/lib/bosh/stemcell/build_environment.rbstemcell_builder/stages/bosh_go_agent/apply.sh
| settings = proxy_settings_from_environment | ||
| .merge(build_time_settings) | ||
| .merge(passthrough_settings_from_environment) | ||
| "env #{hash_as_bash_env(settings)}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add specs for the new passthrough merge path.
The command_env behavior changed but current specs don’t cover BOSH_AGENT_BIN_PATH presence/absence/casing, which makes regressions likely in this new flow.
✅ Suggested spec additions
# in bosh-stemcell/spec/bosh/stemcell/build_environment_spec.rb (describe '#command_env')
context 'when BOSH_AGENT_BIN_PATH is set' do
let(:env) { { 'BOSH_AGENT_BIN_PATH' => '/tmp/bosh-agent' } }
it 'includes BOSH_AGENT_BIN_PATH in command env' do
expect(subject.command_env).to include("BOSH_AGENT_BIN_PATH='/tmp/bosh-agent'")
end
end
context 'when BOSH_AGENT_BIN_PATH is not set' do
let(:env) { {} }
it 'does not include BOSH_AGENT_BIN_PATH' do
expect(subject.command_env).not_to include('BOSH_AGENT_BIN_PATH=')
end
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bosh-stemcell/lib/bosh/stemcell/build_environment.rb` around lines 103 - 106,
Add unit specs for the new passthrough merge path to cover BOSH_AGENT_BIN_PATH
presence, absence and casing for the command_env behavior: in the existing
describe for `#command_env` add contexts that set the subject's env to include
'BOSH_AGENT_BIN_PATH' (e.g. '/tmp/bosh-agent') and assert command_env includes
"BOSH_AGENT_BIN_PATH='/tmp/bosh-agent'"; another context with env = {} asserting
command_env does not include 'BOSH_AGENT_BIN_PATH='; and an additional context
that verifies behavior for alternate casing (e.g. 'bosh_agent_bin_path' or
'Bosh_Agent_Bin_Path') to document whether
passthrough_settings_from_environment/hash_as_bash_env treats keys
case-sensitively. Ensure these specs exercise command_env,
passthrough_settings_from_environment and hash_as_bash_env so regressions are
caught.
| def passthrough_settings_from_environment | ||
| keep = %w(BOSH_AGENT_BIN_PATH) | ||
|
|
||
| environment.select { |k| keep.include?(k.upcase) } | ||
| end |
There was a problem hiding this comment.
Normalize passthrough key casing to uppercase before emitting env.
Line 241 currently preserves caller key casing. If input is bosh_agent_bin_path, it will be emitted lowercase, but downstream apply.sh checks BOSH_AGENT_BIN_PATH uppercase only, so the custom-agent path is skipped unexpectedly.
🐛 Proposed fix
def passthrough_settings_from_environment
- keep = %w(BOSH_AGENT_BIN_PATH)
-
- environment.select { |k| keep.include?(k.upcase) }
+ value = environment['BOSH_AGENT_BIN_PATH']
+ if value.nil?
+ pair = environment.find { |k, _v| k.upcase == 'BOSH_AGENT_BIN_PATH' }
+ value = pair&.last
+ end
+
+ value.nil? ? {} : { 'BOSH_AGENT_BIN_PATH' => value }
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def passthrough_settings_from_environment | |
| keep = %w(BOSH_AGENT_BIN_PATH) | |
| environment.select { |k| keep.include?(k.upcase) } | |
| end | |
| def passthrough_settings_from_environment | |
| value = environment['BOSH_AGENT_BIN_PATH'] | |
| if value.nil? | |
| pair = environment.find { |k, _v| k.upcase == 'BOSH_AGENT_BIN_PATH' } | |
| value = pair&.last | |
| end | |
| value.nil? ? {} : { 'BOSH_AGENT_BIN_PATH' => value } | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bosh-stemcell/lib/bosh/stemcell/build_environment.rb` around lines 238 - 242,
passthrough_settings_from_environment currently returns environment keys with
their original casing which can emit lowercase keys (e.g., bosh_agent_bin_path)
and break downstream checks; update passthrough_settings_from_environment so it
emits keys normalized to uppercase (e.g., BOSH_AGENT_BIN_PATH) while retaining
their values: when selecting keys from environment (using the existing keep list
and environment.select), transform the selected key names to upcase before
returning so apply.sh and other consumers will see the expected uppercase
variable names.
cf-foundation-community-automation
bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
rkoster
left a comment
rkoster
left a comment
There was a problem hiding this comment.
Don't like how intrusive these changes are. For iterating on an agent change I would highly recommend using the repack helper scripts here: https://github.com/cloudfoundry/bosh-linux-stemcell-builder/tree/ubuntu-jammy/scripts/repack-helpers
When working with the docker cpi a simple Dockerfile and the lite stemcell support (which just need a reference to a local image) also work really well.
3 participants
Set BOSH_AGENT_BIN_PATH=./bosh-agent to provide a bosh agent binary to be injected into the stemcell being built. Useful when doing agent development work.
I don't love environment variables, but since this is only a development flow it doesn't seem like it makes much sense as a rake task argument.