Convert SCP-style URLs (no explicit scheme) into proper SSH URLs #1061
Conversation
📝 WalkthroughWalkthroughThe changes involve a significant refactoring of the Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (5)
internal/exec/go_getter_utils.go (5)
15-15: Consider using a more descriptive alias
Short aliases likelmight hamper clarity. Renaming tologorloggercould improve readability.
70-70: Suggest clarifying the field name
sourcecould be named more explicitly, such assourceURIororiginalSource, for improved code clarity.
90-112: Add optional support for custom ports
SCP-style URLs sometimes specify a custom port (e.g.,git@host:port/repo). The current regex won’t match those.- scpPattern := regexp.MustCompile(`^(([\w.-]+)@)?([\w.-]+\.[\w.-]+):([\w./-]+)(\.git)?(.*)$`) + scpPattern := regexp.MustCompile(`^(([\w.-]+)@)?([\w.-]+\.[\w.-]+)(:[0-9]+)?:([\w./-]+)(\.git)?(.*)$`)
136-138: Convert TBC comment into a TODO
Documenting pending enhancements is good. Consider adding a// TODO:or opening an issue.Would you like me to open an issue for broadening token injection support?
139-164: Ensure consistent config toggles
Only GitHub token injection is governed byInjectGithubToken. Consider adding similar toggles for Bitbucket and GitLab for uniformity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/go_getter_utils.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-macos] examples/demo-vendoring
- GitHub Check: [mock-macos] examples/demo-context
- GitHub Check: [mock-macos] examples/demo-component-versions
- GitHub Check: [mock-macos] examples/demo-atlantis
- GitHub Check: [mock-windows] examples/demo-context
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: [mock-windows] examples/demo-atlantis
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-vendoring
- GitHub Check: [mock-linux] examples/demo-context
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Docker Lint
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (11)
internal/exec/go_getter_utils.go (11)
11-11: Looks good
No immediate concerns. Theregexpimport is necessary for the new SCP-style detection.
61-61: Confirmed
Addinggit::sshto the list of valid schemes aligns with go-getter usage.
66-68: Renaming is consistent
This rename clarifies support for multiple Git hosting platforms.
81-88: Excellent documentation
The inline comments clearly explain how SCP-style URLs are handled.
115-115: Duplicate concern: potential credential exposure
Similar to the previous logging ofsrc, sensitive info may be leaked.
123-126: Clear error handling
Returning an error when no SSH agent is available is straightforward.
128-133: Non-standard host scenario well-handled
Skipping token injection for unrecognized hosts is logical.
166-175: Token injection logic is robust
Injecting the token only if credentials aren’t already present is correct.
177-185: Subdir detection
Appending//.for top-level repos is a known go-getter approach. Looks good.
187-197: Default shallow clone
Specifyingdepth=1improves performance but may break use cases needing full history. Confirm this suits your workflows.
208-208: Registering the new CustomGitDetector
Ensuring the custom detector runs first is correct.
|
To test vendoring of SSH style URLs, without SSH key, test instead |
|
Windows tests are failing: === RUN TestCLICommands/atmos_vendor_pull_(no_tty)
cli_test.go:901: Stderr diff mismatch for "D:\\a\\atmos\\atmos\\tests\\snapshots\\TestCLICommands_atmos_vendor_pull_(no_tty).stdout.golden":
--- expected
+++ actual
@@ -1,7 +1,12 @@
INFO Vendoring from 'vendor.yaml'
WARN No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.
-INFO ✓ github/stargazers (main)
-INFO ✓ weather (main)
-INFO ✓ ipinfo (main)
+ERRO Failed to vendor github/stargazers: error : failed to download package: subdir "examples%5Cdemo-library%5Cgithub%5Cstargazers" not found
+INFO x github/stargazers (main)
+ERRO Failed to vendor weather: error : failed to download package: subdir "examples%5Cdemo-library%5Cweather" not found
+INFO x weather (main)
+ERRO Failed to vendor ipinfo: error : failed to download package: subdir "examples%5Cdemo-library%5Cipinfo" not found
+INFO x ipinfo (main)
+INFO Vendored 0 components. Failed to vendor 3 components.
+ |
Have added the test, but it is in disabled state now as current dry run implementation in internal\exec\vendor_component_utils.go exits before hitting custom detectors and go getters. Let me know there are any ojectiosn to adjust vendor_component_utils.go code for the dry run to operate differently. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test-cases/demo-vendoring.yaml (1)
58-59: Clean up YAML formatting.Fix the following formatting issues:
- Line 58: Remove trailing spaces
- Line 59: Add newline at end of file
- - "No SSH authentication method found" - exit_code: 0 \ No newline at end of file + - "No SSH authentication method found" + exit_code: 0 +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 58-58: trailing spaces
(trailing-spaces)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exec/go_getter_utils.go(3 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden(1 hunks)tests/test-cases/demo-vendoring.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/go_getter_utils.go
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/demo-vendoring.yaml
[error] 58-58: trailing spaces
(trailing-spaces)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
tests/test-cases/demo-vendoring.yaml (1)
43-59: LGTM! Well-structured test case for SSH URL handling.The test case effectively validates:
- SSH-style URL handling
- Dry-run behavior
- Error messaging for missing SSH authentication
- Debug logging output
This aligns perfectly with the PR objectives for SCP-style URL support.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 58-58: trailing spaces
(trailing-spaces)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/go_getter_utils.go (2)
81-118: Strong implementation of SCP-style URL handling.The regex pattern and transformation logic effectively handle SCP-style Git URLs while maintaining security through URL masking.
However, consider adding support for IPv6 hosts in the SCP pattern:
-scpPattern := regexp.MustCompile(`^(([\w.-]+)@)?([\w.-]+\.[\w.-]+):([\w./-]+)(\.git)?(.*)$`) +scpPattern := regexp.MustCompile(`^(([\w.-]+)@)?(\[[:0-9a-fA-F]+\]|[\w.-]+\.[\w.-]+):([\w./-]+)(\.git)?(.*)$`)
152-209: Comprehensive token injection implementation.The token injection logic effectively handles multiple Git hosting services with proper fallback mechanisms.
Consider adding Azure DevOps support:
switch host { +case "dev.azure.com": + tokenSource = "AZURE_DEVOPS_TOKEN" + token = os.Getenv(tokenSource) + if token == "" { + tokenSource = "ATMOS_AZURE_DEVOPS_TOKEN" + token = os.Getenv(tokenSource) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/go_getter_utils.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/exec/go_getter_utils.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: internal/exec/go_getter_utils.go:74-75
Timestamp: 2025-02-13T07:30:28.946Z
Learning: In the `CustomGitDetector.Detect` method of `internal/exec/go_getter_utils.go`, verbose debug logging of raw URLs is intentionally kept for debugging purposes, despite potential credential exposure risks.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/go_getter_utils.go (5)
11-15: LGTM! Good choice of logging library.The addition of the
regexppackage andcharmbracelet/loglibrary enhances the code's capabilities for URL parsing and structured logging.
66-71: LGTM! Clear and descriptive type definition.The renaming from
CustomGitHubDetectortoCustomGitDetectorbetter reflects its broader scope of handling multiple Git hosting services.
136-142: Good defensive programming with SSH agent check.The SSH agent check with debug logging helps users identify potential authentication issues early.
224-234: LGTM! Efficient shallow clone configuration.Setting
depth=1by default is a good optimization for reducing bandwidth and improving clone speed.
235-242:❓ Verification inconclusive
Verify URL masking error handling.
While the URL masking logic is good, the error case only logs but doesn't propagate the error.
🏁 Script executed:
#!/bin/bash # Check if there are any other instances where URL masking errors are handled similarly rg -A 2 "MaskBasicAuth.*err.*:=" .Length of output: 36
URL Masking Error Handling Verification
In
internal/exec/go_getter_utils.go(lines 235–242), the error returned fromu.MaskBasicAuthis logged without being propagated. Our grep search didn't reveal additional instances where this pattern appears, so it's unclear whether this behavior is intentional or if error propagation is expected for consistency. Please manually verify if propagating the error here (or elsewhere) would better serve the error-handling strategy across the codebase.
- Verify if propagating the error instead of only logging it is consistent with the rest of the project’s error-handling approach.
- Confirm that this pattern isn't isolated to this function and that similar operations handle errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml (2)
6-13: Terraform Components Configuration Added.
The components section clearly configures Terraform settings such as the base path, auto-approval, and initialization behavior, which enhances clarity in component management. Note: There are trailing spaces detected on line 13; please remove them for cleaner formatting.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
26-30: Remove Excessive Blank Lines.
There are several consecutive blank lines at the end of the file (notably around line 29), which might affect readability. Please remove the extra blank lines.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 29-29: too many blank lines
(5 > 0) (empty-lines)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 29-29: too many blank lines
(5 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml (3)
1-3: New Base Path and Settings Section.
The configuration now defines a base path and enables GitHub token injection. This aligns well with the PR’s objectives for handling token-driven authentication.
14-21: Stacks Configuration Section.
The stacks section is neatly defined with a base path, inclusion/exclusion patterns, and a naming convention. This modular approach will help maintain consistency across environments.
22-25: Log Configuration Added.
The logs section sets the output file and log level explicitly, which should improve troubleshooting and log management.
aknysh
added
minor
|
@Listener430 please review the failing tests |
...napshots/TestCLICommands_atmos_vendor_pull_custom_detector_credentials_leakage.stderr.golden
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@aknysh done, please kindly take a look. |
what
Done:
This is a spin off of #984 that futher extends custom detector logic
Testing
non-standard SCP-style links handling
Token injections were tested wtih bitbucket and gitlab (http) for private and public repos + ssh vendoring for both.
Listing them here as there are no dedicated tests/repos available for testing at bitbucket/gitlab.
why
git::[email protected]:cloudposse/terraform-null-label.git?ref={{.Version}}references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor