Convert SCP-style URLs into proper SSH URLs v2 #1149
Conversation
Listener430
added
minor
Listener430
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
aknysh
added
no-release
📝 WalkthroughWalkthroughThis pull request updates the configuration and functionality of the Atmos vendoring process. It adjusts the boolean syntax in the Codecov configuration file, enhances the custom Git detector with support for multiple Git hosting services and thread safety, and introduces several new helper methods for URL handling. Additional test files verify these changes along with improvements in SCP URL rewriting, scheme validation, and path normalization. New YAML configuration files and snapshot logs for vendoring scenarios, as well as documentation updates regarding SSH and HTTPS vendoring and environment variables, are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Detector as CustomGitDetector
participant Resolver as resolveToken
Client->>Detector: Call Detect(src, _)
Detector->>Detector: Call ensureScheme(src)
Detector->>Detector: Call normalizePath(parsedURL)
Detector->>Resolver: Invoke resolveToken(host)
Resolver-->>Detector: Return token and username
Detector->>Detector: Call injectToken(parsedURL, host)
Detector-->>Client: Return modified URL, status, error
Assessment against linked issues
✨ Finishing Touches
🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (16)
tests/fixtures/scenarios/vendor-creds-sanitize/atmos.yaml (3)
1-2: Blank Line Caution at Start
A leading blank line appears at the top (line 1). Although not critical, removing it would satisfy YAMLlint’s warning about excessive blank lines.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
13-13: Trailing Whitespace Detected
There are trailing spaces on line 13. Please remove them to clean up the YAML.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
20-20: Missing Newline at EOF
The file does not end with a newline on line 20. Adding a newline will ensure compliance with best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml (2)
17-18: Minor Typos in Comments
There are a couple of minor typos in the comment text: "vednoring" should be "vendoring" and "spcified" should be "specified". Fixing these will improve clarity.
24-24: Newline Missing at EOF
The file is missing a newline at the end (line 24). Please add one to adhere to best practices and YAMLlint recommendations.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml (1)
1-20: Configuration File Structure is Solid
The structure and settings in this YAML file are clear and well-organized. Please add a newline at the end (line 20) to resolve the YAMLlint warning.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
tests/fixtures/scenarios/vendor-creds-sanitize/vendor.yaml (2)
18-18: Trailing Whitespace Notice
There are trailing spaces on line 18. Removing these will help adhere to formatting best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: trailing spaces
(trailing-spaces)
35-35: EOF Newline Missing
The file does not end with a newline character on line 35. Please add a newline for consistency with file formatting guidelines.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 35-35: no new line character at the end of file
(new-line-at-end-of-file)
tests/test-cases/vendoring-ssh-dryrun.yaml (1)
22-40: Good security test for credential leakage prevention.This test case effectively validates that sensitive credentials are never leaked in logs, which is a critical security requirement. The explicit assertions using
!notensure that neither the token value nor the environment variable name appear in the output.However, there are two minor formatting issues to fix:
- !not 'supersecret' - - !not 'ATMOS_GITHUB_TOKEN' + - !not 'ATMOS_GITHUB_TOKEN' +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 38-38: trailing spaces
(trailing-spaces)
[error] 40-40: no new line character at the end of file
(new-line-at-end-of-file)
website/docs/cli/configuration/configuration.mdx (1)
699-701: Correct minor documentation typo.Kindly fix "comamnds" → "commands."
- The name of the Terraform workspace in which Terraform comamnds should be run + The name of the Terraform workspace in which Terraform commands should be runwebsite/docs/cli/commands/vendor/vendor-pull.mdx (1)
111-227: Consider replacing triple dots with a typographical ellipsis.In line references like
github.com/..., using "…" (ellipsis) can be a stylistic enhancement.- git::https://github.com/... + git::https://github.com/…🧰 Tools
🪛 LanguageTool
[style] ~216-~216: Consider using a different verb to strengthen your wording.
Context: ...lved When resolving Git sources, Atmos follows these rules: 1. If a *full HTTPS URL...(FOLLOW_OBEY)
[style] ~218-~218: Consider using the typographical ellipsis character here instead.
Context: ...1. If a full HTTPS URL is provided (git::https://github.com/...), it is used as-is. No token data is i...(ELLIPSIS)
internal/exec/go_getter_utils_test.go (5)
63-81: Good test for scheme handling but add negative test case.The test covers preservation, SCP rewriting, and defaulting behavior. Consider adding a test with an invalid or explicitly disallowed scheme to verify rejection behavior.
+ func TestEnsureScheme_InvalidScheme(t *testing.T) { + config := fakeAtmosConfig(false) + detector := &CustomGitDetector{AtmosConfig: config} + in := "ftp://example.com/repo.git" + out := detector.ensureScheme(in) + if strings.HasPrefix(out, "ftp://") { + t.Errorf("Expected invalid FTP scheme to be replaced, but got %s", out) + } + }
177-210: File I/O test is thorough but needs error checks.The test covers the basic functionality well, but doesn't check for errors in important places like the file read operation.
data, err := os.ReadFile(destFile) if err != nil { - t.Errorf("Error reading downloaded file: %v", err) + t.Fatalf("Error reading downloaded file: %v", err) }
212-234: Verify parsed format explicitly.The test confirms the content but doesn't explicitly verify that JSON format detection worked correctly. Consider adding an assertion that confirms the format was detected as JSON.
if resMap["key"] != "value" { t.Errorf("Expected key to be 'value', got %v", resMap["key"]) } + // Verify the file was detected as JSON format + if format := detectFormat(testFile); format != "json" { + t.Errorf("Expected format to be detected as 'json', got %v", format) + }
247-253: Consider edge case for URL rewriting.The test checks for non-SCP URLs, but consider adding a test for edge cases like URLs with a user but no host, or with special characters in the user/host parts.
+ func TestRewriteSCPURL_EdgeCases(t *testing.T) { + cases := []struct { + input string + expected bool + desc string + }{ + {"user@:", false, "missing host"}, + {"git@host.com/no-colon-path", false, "missing colon separator"}, + {"git@host.com:", false, "missing path after colon"}, + } + + for _, tc := range cases { + _, rewritten := rewriteSCPURL(tc.input) + if rewritten != tc.expected { + t.Errorf("%s: expected rewritten=%v, got %v for input %s", + tc.desc, tc.expected, rewritten, tc.input) + } + } + }
1-274: Consider adding unit tests for more edge cases.The test suite is generally thorough, but could benefit from additional test cases covering:
- Network errors during download
- Malformed JSON/YAML handling
- Rate limiting or authentication failures
- Timeouts
These would improve robustness against real-world failure conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
codecov.yml(1 hunks)internal/exec/go_getter_utils.go(4 hunks)internal/exec/go_getter_utils_test.go(1 hunks)internal/exec/vendor_utils.go(2 hunks)internal/exec/yaml_func_include.go(1 hunks)pkg/utils/url_utils.go(1 hunks)pkg/utils/url_utils_test.go(1 hunks)tests/fixtures/scenarios/vendor-creds-sanitize/atmos.yaml(1 hunks)tests/fixtures/scenarios/vendor-creds-sanitize/vendor.yaml(1 hunks)tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml(1 hunks)tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_custom_detector_credentials_leakage.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden(1 hunks)tests/test-cases/vendoring-ssh-dryrun.yaml(1 hunks)website/docs/cli/commands/vendor/vendor-pull.mdx(2 hunks)website/docs/cli/configuration/configuration.mdx(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden (2)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-24T21:34:05.038Z
Learning: Test snapshots in the Atmos project, particularly for dry run scenarios, may be updated during the development process, and temporary inconsistencies in their content should not be flagged as issues.
Learnt from: Listener430
PR: cloudposse/atmos#1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-24T21:34:12.311Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden (2)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-24T21:34:05.038Z
Learning: Test snapshots in the Atmos project, particularly for dry run scenarios, may be updated during the development process, and temporary inconsistencies in their content should not be flagged as issues.
Learnt from: Listener430
PR: cloudposse/atmos#1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-24T21:34:12.311Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
tests/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden (2)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-24T21:34:05.038Z
Learning: Test snapshots in the Atmos project, particularly for dry run scenarios, may be updated during the development process, and temporary inconsistencies in their content should not be flagged as issues.
Learnt from: Listener430
PR: cloudposse/atmos#1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-24T21:34:12.311Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
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-03-24T21:34:12.311Z
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.
🧬 Code Definitions (4)
internal/exec/yaml_func_include.go (1)
internal/exec/go_getter_utils.go (1)
DownloadDetectFormatAndParseFile(362-376)
internal/exec/vendor_utils.go (1)
pkg/utils/file_utils.go (1)
TrimBasePathFromPath(83-87)
pkg/utils/url_utils_test.go (1)
pkg/utils/url_utils.go (1)
MaskBasicAuth(8-24)
internal/exec/go_getter_utils.go (1)
pkg/utils/url_utils.go (1)
MaskBasicAuth(8-24)
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/vendor-creds-sanitize/atmos.yaml
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
tests/fixtures/scenarios/vendor-creds-sanitize/vendor.yaml
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 35-35: no new line character at the end of file
(new-line-at-end-of-file)
tests/test-cases/vendoring-ssh-dryrun.yaml
[error] 38-38: trailing spaces
(trailing-spaces)
[error] 40-40: no new line character at the end of file
(new-line-at-end-of-file)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: codecov/patch
internal/exec/yaml_func_include.go
[warning] 50-50: internal/exec/yaml_func_include.go#L50
Added line #L50 was not covered by tests
internal/exec/go_getter_utils.go
[warning] 87-89: internal/exec/go_getter_utils.go#L87-L89
Added lines #L87 - L89 were not covered by tests
[warning] 98-98: internal/exec/go_getter_utils.go#L98
Added line #L98 was not covered by tests
[warning] 118-118: internal/exec/go_getter_utils.go#L118
Added line #L118 was not covered by tests
[warning] 174-175: internal/exec/go_getter_utils.go#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 178-179: internal/exec/go_getter_utils.go#L178-L179
Added lines #L178 - L179 were not covered by tests
[warning] 181-181: internal/exec/go_getter_utils.go#L181
Added line #L181 was not covered by tests
[warning] 207-208: internal/exec/go_getter_utils.go#L207-L208
Added lines #L207 - L208 were not covered by tests
[warning] 224-239: internal/exec/go_getter_utils.go#L224-L239
Added lines #L224 - L239 were not covered by tests
[warning] 257-258: internal/exec/go_getter_utils.go#L257-L258
Added lines #L257 - L258 were not covered by tests
[warning] 339-340: internal/exec/go_getter_utils.go#L339-L340
Added lines #L339 - L340 were not covered by tests
[warning] 350-351: internal/exec/go_getter_utils.go#L350-L351
Added lines #L350 - L351 were not covered by tests
🪛 LanguageTool
website/docs/cli/commands/vendor/vendor-pull.mdx
[style] ~216-~216: Consider using a different verb to strengthen your wording.
Context: ...lved When resolving Git sources, Atmos follows these rules: 1. If a *full HTTPS URL...
(FOLLOW_OBEY)
[style] ~218-~218: Consider using the typographical ellipsis character here instead.
Context: ...1. If a full HTTPS URL is provided (git::https://github.com/...), it is used as-is. No token data is i...
(ELLIPSIS)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
🔇 Additional comments (26)
codecov.yml (1)
16-16: Standardized boolean value in codecov configuration.The change from
yestotrueimproves consistency by using proper YAML boolean syntax rather than string representations. This is a good practice.internal/exec/yaml_func_include.go (1)
50-50:✅ Verification successful
Pass atmosConfig by reference instead of by value.
The change correctly updates the parameter passing to match the updated signature of
DownloadDetectFormatAndParseFilewhich now expects a pointer. This is more efficient for large structures and maintains consistency with the function's implementation.
🏁 Script executed:
#!/bin/bash # Check if the atmosConfig parameter is consistently passed as a pointer elsewhere echo "Checking for other calls to DownloadDetectFormatAndParseFile:" rg "DownloadDetectFormatAndParseFile\(" --type go -A 1 -B 1Length of output: 1271
Consistent Use of Pointer for atmosConfig
The updated call to
res, err = DownloadDetectFormatAndParseFile(&atmosConfig, f)now properly passes the configuration as a pointer, aligning with the revised function signature. The verification shows that this pointer usage is consistent across the codebase (for instance, in both the utility and test files). Great work ensuring efficiency and consistency!
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 50-50: internal/exec/yaml_func_include.go#L50
Added line #L50 was not covered by teststests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden (1)
1-10: Test snapshot captures expected CLI output.This snapshot provides test validation for the SSH vendoring functionality, capturing debug logs, warnings about TTY detection, and confirmation of the dry run process.
Based on previous learnings, I understand that the "terrafrom-null-label" typo on line 7 is intentional as it captures the exact output of the command.
pkg/utils/url_utils.go (1)
8-24:✅ Verification successful
New URL credential masking function improves security.
This utility function effectively masks authentication credentials in URLs by replacing usernames and passwords with "xxx". The implementation properly handles different scenarios (username-only, username+password) and includes appropriate error handling.
🏁 Script executed:
#!/bin/bash # Verify if there are unit tests for this new function echo "Checking for unit tests of MaskBasicAuth function:" rg "func Test.*MaskBasicAuth" --type goLength of output: 486
Great work on the MaskBasicAuth function!
The implementation inpkg/utils/url_utils.go(lines 8-24) robustly masks URL credentials (both username-only and username+password scenarios) and handles parsing errors appropriately. The unit tests inpkg/utils/url_utils_test.goconfirm that all relevant scenarios—including invalid URLs—are correctly covered.tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden (1)
1-13: Snapshot Output Looks Consistent
The snapshot output accurately captures the command’s debug and informational messages. As these files serve to verify exact outputs, intentional discrepancies (like minor typos) should remain as-is.tests/fixtures/scenarios/vendor-creds-sanitize/vendor.yaml (1)
10-12: Ensure URL Conversion for Scheme-less Sources
The first source’s URL on line 12 is missing an explicit scheme (e.g., "https://"). Since this PR focuses on converting non-standard SCP-style URLs, please verify that the custom detector correctly transforms such entries into valid URLs.tests/snapshots/TestCLICommands_atmos_vendor_pull_custom_detector_credentials_leakage.stderr.golden (1)
7-11: Good security practice with credential masking.The log lines show proper masking of sensitive credentials in line 10 where the GitHub token is masked as "xxx:xxx", preventing accidental exposure of secrets in logs. This aligns with the PR's goal of addressing credentials leakage in debug statements.
internal/exec/vendor_utils.go (2)
22-23: Good addition of a dedicated stderr logger.Creating a dedicated logger for stderr is a clean approach to separate detailed debugging output from standard output. This improves the clarity of terminal output during operations.
514-514: Improved logging structure with field-based approach.The updated logging now uses the new stderr logger and implements a field-based approach (
"path", value) which is more structured and consistent than the previous format. This makes logs easier to parse and filter.tests/test-cases/vendoring-ssh-dryrun.yaml (1)
3-21: Well-structured test for SSH URL vendoring.This test case properly validates the SSH URL handling capabilities being added in this PR. The test includes appropriate environment setup and verification criteria to ensure the functionality works as expected in dry-run mode.
pkg/utils/url_utils_test.go (4)
7-17: Good test coverage for credential masking with username and password.This test properly validates that URLs containing both username and password get properly masked, which is essential for the PR's goal of preventing credential leakage.
19-29: Comprehensive test for username-only scenarios.Good inclusion of a test case that handles URLs with only a username but no password. This ensures the masking function works correctly with different authentication patterns.
31-41: Edge case coverage for URLs without credentials.This test ensures the function doesn't modify URLs that don't contain credentials, preventing unintended side effects when processing regular URLs.
43-49: Proper error handling test for invalid URLs.Good defensive programming practice to test error conditions. This ensures the function correctly handles malformed URLs rather than failing silently or producing unexpected behavior.
tests/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden (1)
1-16: Snapshot log looks good.All debug messages appear intentionally masked (e.g.,
xxx:xxx), which is commendable. Nothing problematic observed.website/docs/cli/configuration/configuration.mdx (1)
681-683: The newly documented environment variables are clear and useful.Providing explicit variables for Bitbucket and GitLab helps users avoid rate limits and ensures seamless authentication.
internal/exec/go_getter_utils.go (4)
66-71: Struct documentation and field additions look correct.The
CustomGitDetectorstruct along with its doc comment is clear and aligns with your multi-host approach.
339-340: Symlink removal approach repeated.This duplicates previous discussions about partial removal failures (referenced in #984).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 339-340: internal/exec/go_getter_utils.go#L339-L340
Added lines #L339 - L340 were not covered by tests
350-351: Partial removal failures also noted previously.This is out of scope for this PR, matching prior feedback in #984.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 350-351: internal/exec/go_getter_utils.go#L350-L351
Added lines #L350 - L351 were not covered by tests
174-179:✅ Verification successful
Test rewriting SCP-style URLs with default username injection.
Including a scenario where an empty user triggers "git@" insertion will confirm correctness.
🏁 Script executed:
#!/bin/bash rg "rewriteSCPURL" -A 5Length of output: 2122
SCP URL rewriting and default username injection verified
The tests ininternal/exec/go_getter_utils_test.goconfirm that SCP-style URLs are correctly rewritten to start with"ssh://"and that, in cases of an empty username, the default"git@"is injected as expected. No further action is needed.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 174-175: internal/exec/go_getter_utils.go#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 178-179: internal/exec/go_getter_utils.go#L178-L179
Added lines #L178 - L179 were not covered by testsinternal/exec/go_getter_utils_test.go (6)
16-16: Good pattern for preserving test isolation.Saving the original detectors at the beginning ensures you can restore them after test execution, preventing test pollution.
18-24: Helper function simplifies test setup.This concise helper function creates test configurations without duplicating code. Consider adding more test cases with different configuration parameters to increase coverage.
26-49: Solid URI validation test coverage.Your test includes both positive and negative test cases. The validation of empty URIs, long URIs, path traversal, and spaces is thorough.
149-175: Good symlink test pattern.The test properly creates a controlled environment, tests functionality, and cleans up afterward. Skipping on Windows is appropriate given platform differences.
255-268: Good test for handling invalid URL paths.Testing that an invalid percent encoding in a URL path doesn't cause a panic is important. The test effectively verifies that the function handles this edge case gracefully.
270-274: TestMain ensures proper cleanup.Restoring the original detectors in TestMain is the right approach to prevent test pollution. This ensures that tests don't affect each other or the global state.
|
💥 This pull request now has conflicts. Could you fix it @Listener430? 🙏 |
|
@Listener430 please resolve the conflicts |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/utils/url_utils.go (1)
10-26: Consider validating empty URLs before parsing.The function currently tries to parse any string passed to it. Adding a check for empty strings could prevent unnecessary error handling.
func MaskBasicAuth(rawURL string) (string, error) { + if rawURL == "" { + return "", nil + } parsedURL, err := url.Parse(rawURL) if err != nil { return "", fmt.Errorf("failed to parse URL: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/exec/copy_glob.go(1 hunks)pkg/utils/url_utils.go(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/exec/copy_glob.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden
- tests/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/utils/url_utils.go (2)
8-9: Excellent addition of a standardized masking constant.Using a constant here improves clarity and ensures consistency across the codebase when masking sensitive data.
17-22: The improved masking logic is solid.The conditional approach properly handles different authentication scenarios:
- Both username and password get masked when both exist
- Only username gets masked when that's all that's present
This is a clean implementation that follows Go best practices.
@aknysh thanks, done, please kindly take a look. |
|
These changes were released in v1.168.0. |
what
This is a replacement of #1061 (that got too big, thus v2 in the name) to address autofix (fixed already) and test coverage issue (fixed).
Done:
This is a spin off of #984 that futher extends custom detector logic
Testing
Use this to run only test cases relevant for this PR
$ go test -v -run '^TestCLICommands/(atmos_vendor_pull_using_SSH|atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage)$' github.com/cloudposse/atmos/testsnon-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::git@github.com:cloudposse/terraform-null-label.git?ref={{.Version}}references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation