File Settings should return error in case the file itself does not exist #437
File Settings should return error in case the file itself does not exist #437fmoehler wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughPublicSSHKeyForUsername now checks whether the settings file exists via s.fs.FileExists and returns an error "Settings file '' does not exist" if missing; otherwise it continues to return the previous result. Tests were split into contexts covering the settings-file-present and settings-file-missing cases. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infrastructure/file_settings_source.go`:
- Line 38: Update the spec in infrastructure/file_settings_source_test.go to
expect an error from the File Setting Source call that previously returned an
SSH key: change the assertion that currently expects err == nil to assert err !=
nil (and optionally assert the error message contains "does not provide ssh key"
or matches bosherr.Error output). Locate the test that invokes the File Setting
Source method (the call that previously returned the SSH key) and replace the
nil-error expectation with a non-nil assertion and corresponding message check
so the test verifies the new error contract.
🪄 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: c99ca351-1668-46b8-ae45-62925b316c43
📒 Files selected for processing (1)
infrastructure/file_settings_source.go
fmoehler
force-pushed
the
file-setting-should-return-error-for-ssh-key
branch
from
a2f5286 to
0b2bbfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
infrastructure/file_settings_source_test.go (1)
29-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest expects incorrect behavior.
This test expects
PublicSSHKeyForUsernameto return no error when the settings file exists (line 39), but that contradicts the PR objective. The file settings source should always return an error when an SSH key is requested (because it doesn't provide SSH keys), allowing the agent to fall back to other sources. The past review comment flagged this same issue and was marked as addressed, yet the test still validates the wrong contract.Once the implementation is corrected to always return an error, this test case should be removed or replaced with a single test that verifies an error is always returned.
🧪 Proposed fix to test the correct behavior
Replace the two context blocks with a single test that verifies an error is always returned:
Describe("PublicSSHKeyForUsername", func() { - Context("when the settings file exists", func() { - BeforeEach(func() { - settingsFileName := "/fake-settings-file-path" - source = infrastructure.NewFileSettingsSource(settingsFileName, fs, logger) - err := fs.WriteFileString(settingsFileName, "{}") - Expect(err).NotTo(HaveOccurred()) - }) - - It("returns an empty string", func() { - publicKey, err := source.PublicSSHKeyForUsername("fake-username") - Expect(err).ToNot(HaveOccurred()) - Expect(publicKey).To(Equal("")) - }) - }) - - Context("when the settings file does not exist", func() { - BeforeEach(func() { - source = infrastructure.NewFileSettingsSource("/missing-settings-file-path", fs, logger) - }) - - It("returns an error", func() { - _, err := source.PublicSSHKeyForUsername("fake-username") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("File for settings source does not exist")) - }) + BeforeEach(func() { + source = infrastructure.NewFileSettingsSource("/fake-settings-file-path", fs, logger) + }) + + It("always returns an error because file settings source does not provide SSH keys", func() { + _, err := source.PublicSSHKeyForUsername("fake-username") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("File settings source does not provide SSH keys")) }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/file_settings_source_test.go` around lines 29 - 42, The test is asserting the wrong contract: FileSettingsSource should always return an error for SSH key requests. Update the spec around infrastructure.NewFileSettingsSource / PublicSSHKeyForUsername so it no longer expects a successful call; remove the existing "when the settings file exists" block and replace it with a single test that calls source.PublicSSHKeyForUsername("fake-username") and asserts that an error is returned (and optionally that the returned key is empty), ensuring the test matches the intended behavior that FileSettingsSource never supplies SSH keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infrastructure/file_settings_source.go`:
- Around line 37-42: The PublicSSHKeyForUsername method on FileSettingsSource
currently returns ("", nil) when the settings file exists which stops fallback;
change it so it always returns a non-nil error when asked for an SSH key (even
if s.fs.FileExists(s.settingsFilePath) is true). In other words, keep the
existing missing-file error branch, and replace the final return ("", nil) in
FileSettingsSource.PublicSSHKeyForUsername with a return of ("",
bosherr.Error(...)) that clearly states this source does not provide SSH keys so
multi-source fallback can continue.
---
Duplicate comments:
In `@infrastructure/file_settings_source_test.go`:
- Around line 29-42: The test is asserting the wrong contract:
FileSettingsSource should always return an error for SSH key requests. Update
the spec around infrastructure.NewFileSettingsSource / PublicSSHKeyForUsername
so it no longer expects a successful call; remove the existing "when the
settings file exists" block and replace it with a single test that calls
source.PublicSSHKeyForUsername("fake-username") and asserts that an error is
returned (and optionally that the returned key is empty), ensuring the test
matches the intended behavior that FileSettingsSource never supplies SSH keys.
🪄 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: 744de521-026a-4922-9f7e-a576da679b61
📒 Files selected for processing (2)
infrastructure/file_settings_source.goinfrastructure/file_settings_source_test.go
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
fmoehler
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the FileSettingsSource SSH key lookup behavior so that it returns an error when its settings file is missing, allowing MultiSettingsSource to fall back to subsequent sources instead of prematurely “succeeding” with an empty SSH key.
Changes:
- Add a settings-file existence check in
FileSettingsSource.PublicSSHKeyForUsernameand return an error when missing. - Extend
FileSettingsSourceunit tests to cover both “file exists” and “file missing” cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
infrastructure/file_settings_source.go |
Return an error from SSH key lookup when the settings file path doesn’t exist, enabling fallback to other sources. |
infrastructure/file_settings_source_test.go |
Add test coverage for the new “missing settings file returns error” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rkoster
requested review from
a team,
s4heid and
selzoc
and removed request for
a team
|
Seems reasonable to me - probably worth addressing some of the ai-generated comments. Do any other Iaases use the |
fmoehler
force-pushed
the
file-setting-should-return-error-for-ssh-key
branch
from
d07f176 to
0b2bbfb
Compare
- Include settings file path in PublicSSHKeyForUsername error message for easier debugging - Add warning logs in MultiSettingsSource when SSH key retrieval fails from a source - Update tests to verify error message includes file path and logging behavior
|
Hi @selzoc , |
|
Does this PR change the behavior for the Azure case? |
|
Hi @selzoc , |
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
The agent loops over the settings sources defined in the stemcell. E.g. here for openstack when trying to retrieve the default ssh key here. In case of an error the agent tries the next source. The file setting source, however, does not return an SSH Key at all, but also does not error out, therefore blocking the agent from trying other sources for the SSH key retrieval. Therefore, the file setting source should return an error when an ssh key is trying to be fetched, so that the agent can fall back to other settings sources for the ssh key retrieval.