Update dry run for atmos vendor pull to support ssh + detailed SCP urls alignment #1076
Conversation
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Listener430
changed the title
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.
… only in dry mode
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml (2)
9-18: Basic HTTPS Source Configuration Review
The first source entry clearly documents a basic HTTPS URL (with token injection expected), and the structure is correct. Note that line 18 has trailing spaces that should be removed to satisfy YAML lint rules.- # Basic HTTPS default (token injection expected)␣␣ + # Basic HTTPS default (token injection expected)🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: trailing spaces
(trailing-spaces)
19-27: Direct Credentials URL Configuration Caution
The second source entry uses direct credentials in the URL. While this may be intentional for testing or specific scenarios, please verify that exposing credentials in configuration is acceptable for your environment.tests/test-cases/demo-vendoring.yaml (2)
61-79: Environment Variable Formatting Consistency
The "atmos vendor pull ssh component" test case is sound in its intent. However, notice that theATMOS_GITHUB_TOKENvalue is provided without quotes here while quoted in the SSH test case. For maintainability and consistency, consider using the same quoting style for environment variables across test cases.
96-96: Remove Trailing Whitespace
Static analysis has detected trailing spaces on these lines. Removing these extraneous spaces helps maintain clean YAML formatting.- - "Injecting token" + - "Injecting token" - - "!supersecret" + - "!supersecret"Also applies to: 98-98
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 96-96: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/exec/vendor_utils.go(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/test-cases/demo-vendoring.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/exec/vendor_utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/snapshots/TestCLICommands_atmos_vendor_pull_custom_detector_credentials_leakage.stderr.golden
- tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/demo-vendoring.yaml
[error] 96-96: trailing spaces
(trailing-spaces)
[error] 98-98: trailing spaces
(trailing-spaces)
tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml
[error] 18-18: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml (2)
1-8: File Header Configuration Looks Good
The top-level keys such asapiVersion,kind, andmetadata(withnameanddescription) are clearly defined and follow the expected schema for an Atmos vendoring manifest.
28-36: SSH Implicit Source Configuration Validated
The third source entry correctly represents an SSH implicit method, aligning with the enhanced SCP and SSH URL support highlighted in the PR objectives. The structure is consistent with the other components, ensuring that the custom detectors in the codebase can correctly process these URLs.tests/test-cases/demo-vendoring.yaml (2)
43-60: SSH Dry-Run Test Case Validation
This test case correctly exercises the dry-run mode for vendor pulls using an SSH-style URL. The use of detailed logging (via--logs-level=Debug) and the dry-run flag ensures that no actual file downloads occur. The work directory and environment variable setup align with the PR objectives.
80-101: Credential Masking Verification
This test case does an excellent job ensuring that injected credentials do not appear in log outputs by checking for a masked output (e.g., "!supersecret") and an indicative log message ("Injecting token"). The expected stdout and stderr values properly cover the security requirement regarding credential leakage.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 96-96: trailing spaces
(trailing-spaces)
[error] 98-98: trailing spaces
(trailing-spaces)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
tests/fixtures/scenarios/vendor-creds-sanitize/atmos.yaml (1)
13-13: Remove trailing whitespace.There's trailing whitespace on this line that should be removed.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml (1)
1-25: Minor typo in comment.There's a small typo in line 17's comment.
- ## Explicit ssh vednoring (the schema is explicitly spcified along with a username, no custom detector is invoked) + ## Explicit ssh vendoring (the schema is explicitly specified along with a username, no custom detector is invoked)tests/fixtures/scenarios/vendoring-dry-run/atmos.yaml (1)
1-23: Fix YAML formatting issues.There are a few formatting issues in this YAML file that should be addressed.
- base_path: "./" settings: inject_github_token: true components: terraform: base_path: "components/terraform" apply_auto_approve: false deploy_run_init: true init_run_reconfigure: true auto_generate_backend_file: false - + stacks: base_path: "stacks" included_paths: - "deploy/**/*" excluded_paths: - "**/_defaults.yaml" name_pattern: "{stage}" - - +🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 22-22: too many blank lines
(2 > 0) (empty-lines)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml (1)
1-20: Add newline at end of file.The YAML file is missing a newline character at the end.
logs: file: "/dev/stderr" level: Info +🧰 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: Fix trailing whitespaceRemove the trailing spaces at the end of this line.
- # Direct credentials provided in the URL (token injection should be skipped) + # Direct credentials provided in the URL (token injection should be skipped)🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: trailing spaces
(trailing-spaces)
28-35: Add newline at end of fileAdd a newline character at the end of the file to comply with YAML standards.
tags: - demo +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 35-35: no new line character at the end of file
(new-line-at-end-of-file)
website/docs/cli/commands/vendor/vendor-pull.mdx (1)
216-218: Consider refining the wording and punctuation.You might strengthen the verb choice in "When resolving Git sources, Atmos follows these rules" by saying “When processing Git sources, Atmos follows these rules.” Also, using a typographical ellipsis character “…” rather than “...” can be more stylistically consistent.
🧰 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)
tests/test-cases/vendoring-ssh-dryrun.yaml (1)
57-59: Remove trailing spaces and add a newline at EOF.YAMLLint flagged trailing spaces on line 57 and requires a newline at the end of the file on line 59. Please ensure both are fixed:
- !not 'supersecret' - - !not 'ATMOS_GITHUB_TOKEN' \ No newline at end of file + - !not 'ATMOS_GITHUB_TOKEN' +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
website/docs/cli/configuration/configuration.mdx (2)
681-683: Minor grammar fixes.
- Lowercase "Requests" for consistency in “Unauthenticated requests”.
- Possibly clarify “avoided” vs. “used” regarding rate limits.
- | ATMOS_BITBUCKET_TOKEN | N/A | App password for Bitbucket API requests is set to avoid rate limits. Unauthenticated Requests are limited to 60 requests per hour across all API resources. | + | ATMOS_BITBUCKET_TOKEN | N/A | App password for Bitbucket API requests, helping avoid rate limits. Unauthenticated requests are limited to 60 requests per hour across all API resources. |
699-699: Typographical correction.Fix the spelling from “comamnds” to “commands”:
- | ATMOS_TERRAFORM_WORKSPACE | The name of the Terraform workspace in which Terraform comamnds should be run | + | ATMOS_TERRAFORM_WORKSPACE | The name of the Terraform workspace in which Terraform commands should be run |internal/exec/go_getter_utils_test.go (3)
118-137: Token injection test could be more robust.While the test correctly verifies the basic token injection, consider adding cases for empty tokens or different host scenarios to ensure comprehensive coverage.
func TestInjectToken(t *testing.T) { os.Setenv("GITHUB_TOKEN", "testtoken") defer os.Unsetenv("GITHUB_TOKEN") config := fakeAtmosConfig(true) detector := &CustomGitDetector{AtmosConfig: &config} uObj, err := url.Parse("https://github.com/user/repo.git") if err != nil { t.Fatalf("Failed to parse URL: %v", err) } detector.injectToken(uObj, hostGitHub) if uObj.User == nil { t.Error("Expected token to be injected into URL") } else { user := uObj.User.Username() if user != getDefaultUsername(hostGitHub) { t.Errorf("Expected username %s, got %s", getDefaultUsername(hostGitHub), user) } } + + // Test with empty token + os.Unsetenv("GITHUB_TOKEN") + uObj2, _ := url.Parse("https://github.com/user/repo.git") + detector.injectToken(uObj2, hostGitHub) + if uObj2.User != nil { + t.Error("Expected no token injection with empty token") + } + + // Test with non-GitHub host + os.Setenv("GITLAB_TOKEN", "gltoken") + defer os.Unsetenv("GITLAB_TOKEN") + uObj3, _ := url.Parse("https://gitlab.com/user/repo.git") + detector.injectToken(uObj3, hostGitLab) + if uObj3.User == nil { + t.Error("Expected token to be injected for GitLab URL") + } else if uObj3.User.Username() != getDefaultUsername(hostGitLab) { + t.Errorf("Expected username %s, got %s", getDefaultUsername(hostGitLab), uObj3.User.Username()) + } }
300-310: Consider consolidating redundant URI validation tests.These test cases duplicate checks already present in the main TestValidateURI function (lines 37-42). Consider consolidating into a single comprehensive test to improve maintainability.
312-319: Consider consolidating redundant SCP URL tests.This test duplicates functionality already tested in TestRewriteSCPURL (lines 98-102). Consolidating these would make the test suite more maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.github/workflows/test.yml(3 hunks).golangci.yml(1 hunks)codecov.yml(1 hunks)internal/exec/go_getter_utils.go(4 hunks)internal/exec/go_getter_utils_test.go(1 hunks)internal/exec/vendor_model.go(2 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/fixtures/scenarios/vendoring-dry-run/atmos.yaml(1 hunks)tests/fixtures/scenarios/vendoring-dry-run/components/terraform/ipinfo/component.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_component_using_SSH.stderr.golden(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_ssh_component.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 (6)
internal/exec/vendor_model.go (1)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
tests/snapshots/TestCLICommands_atmos_vendor_pull_component_using_SSH.stderr.golden (1)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-12T18:52:22.003Z
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.
tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden (1)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-12T18:52:22.003Z
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.
tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh_component.stderr.golden (1)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-12T18:52:22.003Z
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.
tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden (1)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-12T18:52:22.003Z
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.
internal/exec/go_getter_utils.go (2)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: internal/exec/go_getter_utils.go:74-75
Timestamp: 2025-03-12T18:52:31.814Z
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.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
🪛 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)
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/vendoring-dry-run/atmos.yaml
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 22-22: too many blank lines
(2 > 0) (empty-lines)
tests/test-cases/vendoring-ssh-dryrun.yaml
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 59-59: 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)
🪛 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 (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (42)
.golangci.yml (1)
173-173: Increased nestif complexity thresholdRaising the min-complexity threshold from 4 to 6 will reduce noise in linting output by only flagging more complex nested if statements.
codecov.yml (1)
16-16: Improved boolean type representationChanged from string representation
yesto proper boolean valuetrue. This is a good practice for YAML configuration files.internal/exec/yaml_func_include.go (1)
50-50: Changed parameter passing to use pointerUpdated to pass
atmosConfigby reference instead of by value, which aligns with the function signature ingo_getter_utils.gothat expects a pointer parameter.tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden (1)
1-10: New test snapshot for SSH vendor pull dry runThe snapshot captures the expected CLI output for the vendor pull command when using SSH, including debug logs, warnings, and status messages. This supports the PR objective of enhancing dry run output with more detailed information.
I note this snapshot shows proper sanitized output including detailed information about vendored components without downloading files, which directly addresses the objectives of this PR.
internal/exec/vendor_model.go (2)
15-15: Good job fixing the import alias.The
charmbracelet/logpackage is now correctly imported with thelogalias, following the project's import alias configuration.
337-337: Improved debugging visibility.Adding this debug log provides better visibility into the package installation process, which is valuable for troubleshooting and aligns with the PR's goal to enhance output information.
tests/fixtures/scenarios/vendor-creds-sanitize/atmos.yaml (1)
1-21: Good test configuration for credential sanitization.This configuration file properly sets up a test scenario with GitHub token injection enabled, which will be useful for testing the sanitization features in the dry run output.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
[error] 13-13: trailing spaces
(trailing-spaces)
tests/snapshots/TestCLICommands_atmos_vendor_pull_component_using_SSH.stderr.golden (1)
1-11: Good test snapshot for SSH vendoring.This snapshot correctly captures the expected CLI output for the SSH vendoring dry run scenario, showing proper debug messages, warnings, and confirmation of the dry run completion.
pkg/utils/url_utils.go (2)
8-8: Good constant definition for masking secrets.Using a named constant for the masked value follows good practice and addresses the previous feedback to move the string to a constant.
10-26: Well-implemented URL sanitization function.The
MaskBasicAuthfunction properly handles URL parsing, credential detection, and masking. It correctly differentiates between URLs with username-only and username-password combinations.internal/exec/vendor_utils.go (2)
22-23: LGTM - Adding dedicated stderr logger helps separate detailed messaging.The new StderrLogger ensures that detailed messaging, specifically for files vendoring, doesn't pollute stdout. This aligns well with improving the dry run output clarity.
514-514: Clean implementation of stderr logging.Good replacement of standard log with the dedicated StderrLogger for including paths. This ensures detailed file inclusion messages go to stderr, keeping stdout clean as intended.
tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml (2)
9-16: Good implementation of implicit SSH vendoring.The implicit SSH vendoring configuration correctly uses SCP-style Git URL format (
[email protected]:cloudposse/terraform-null-label.git). This addresses the need for SCP URL alignment in the dry run output as mentioned in the PR objectives.
17-24: Explicit SSH vendoring looks correct.The explicit SSH vendoring configuration uses the proper schema (
ssh://) with a username. This provides a good comparison case to the implicit SSH vendoring and ensures comprehensive testing of SSH support.tests/fixtures/scenarios/vendoring-dry-run/atmos.yaml (1)
4-4: Good addition of GitHub token injection setting.The
inject_github_token: truesetting is important for the vendoring functionality being tested, especially for dry run output that needs to show sanitized credentials.tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml (1)
18-20: Appropriate log configuration for testing.Setting logs to stderr with Info level is a good choice for this scenario. It ensures that logs don't interfere with test outputs while maintaining visibility of important information during tests.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden (1)
11-13: Dry run output structure looks goodThe output clearly shows what would happen in actual execution (vendoring 2 components) after confirming no actual components were vendored during dry run. This separation helps users understand the expected actions without making changes to the filesystem.
tests/fixtures/scenarios/vendor-creds-sanitize/vendor.yaml (1)
9-35: Well-structured test scenarios for credential handlingGood job defining three distinct scenarios for credential handling:
- Default HTTPS with token injection
- Direct credentials in URL (skipping token injection)
- Pre-existing credentials (skipping token injection)
This comprehensive test file will effectively validate the credential sanitization features mentioned in the PR objectives.
🧰 Tools
🪛 YAMLlint (1.35.1)
[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/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden (1)
8-12: Excellent credential handling and loggingThe debug logs provide detailed visibility into the URL transformation process while properly masking sensitive credentials ("xxx:xxx"). This achieves the PR objective of showing detailed information about injected tokens and URL transformations without exposing sensitive data.
tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh_component.stderr.golden (2)
8-12: Successfully implements SCP-style SSH URL rewritingThe logs clearly demonstrate the key feature of this PR - rewriting SCP-style SSH URLs (github.com:cloudposse/atmos.git) to standard SSH URLs (ssh://github.com/cloudposse/atmos.git). The credential injection and masking are also correctly handled, fulfilling the PR's objective of supporting SSH in vendor pull commands.
14-16: Dry run output structure is clearSimilar to the first file, the output effectively separates what actually happened (dry run with no actual vendoring) from what would happen in a real execution (vendoring 1 component). This structure helps users understand the expected actions without making filesystem changes.
.github/workflows/test.yml (3)
49-50: Added full Git history checkout to ensure complete repository access.The change to
fetch-depth: 0ensures the entire Git history is available during workflow execution, which is essential for comprehensive Git-related operations like the vendor pull command with SSH support.
88-88: Increased timeout to allow for more complex Git operations.The timeout increase from 15 to 20 minutes provides adequate time for the enhanced vendor pull functionality to complete successfully, particularly with SSH and URL processing.
159-159: Extended acceptance test timeout to accommodate new vendor pull functionality.Increasing the test timeout to 20 minutes ensures tests have sufficient time to execute, especially when testing various Git URL formats and SSH handling.
tests/snapshots/TestCLICommands_atmos_vendor_pull_custom_detector_credentials_leakage.stderr.golden (3)
7-11: Properly masks credentials in Git URLs during dry run.The logs correctly show the custom Git detector in action with appropriate credential masking. The URL transformation process is well-documented in the logs:
- Detection of Git URL
- Default scheme application
- Token injection
- Credential masking (showing "xxx:xxx" instead of actual tokens)
This is exactly what we want to see for secure credential handling in dry run mode.
32-32: Log entry incorrectly identifies directory as file.The debug message refers to "docs" as a file, but it's actually a directory.
87-92: Clear success indicators for vendored components.The output properly indicates successful vendoring of each component with version information, making it easy to verify the operation's results.
pkg/utils/url_utils_test.go (1)
1-49: Comprehensive test coverage for URL credential masking.The tests thoroughly cover all important scenarios for the MaskBasicAuth function:
- URLs with both username and password
- URLs with username only
- URLs without credentials
- Invalid URL handling
Each test verifies the expected output and proper error handling, ensuring the URL masking functionality is reliable.
tests/fixtures/scenarios/vendoring-dry-run/components/terraform/ipinfo/component.yaml (2)
7-9: SSH URL format correctly implemented for Git source.The configuration properly uses the SSH format (
[email protected]) for the Git URI, which aligns with the PR objective of supporting SSH URLs in the vendor pull command.
10-13: Appropriate file inclusion patterns for Terraform components.The included paths cover all essential Terraform files (.tf), variable files (.tfvars), and documentation (*.md), ensuring that all necessary component files are vendored.
website/docs/cli/commands/vendor/vendor-pull.mdx (2)
111-120: Good addition for SSH vendoring!The new "Vendoring from SSH" section is clear and helps users understand how SSH-based repository access is configured.
150-156: Clarity improvement acknowledged.The explanation about invalid URLs and Atmos defaulting to HTTPS is straightforward and addresses common user pitfalls.
internal/exec/go_getter_utils.go (3)
66-70: Struct definition looks good.This new
CustomGitDetectorneatly encapsulates the Atmos config reference for advanced Git URL handling.
74-124: Useful custom detection logic.The rewriting of SCP-style URLs and the fallback to
https://help unify various URL forms. Masking the source URL in debug logs further protects credentials.
345-359: Symlink removal approach is straightforward.Recursively walking the downloaded directory and removing symlinks reduces security risks. This appears correct for ensuring only genuine files remain after the clone.
internal/exec/go_getter_utils_test.go (7)
30-52: Great job with comprehensive URI validation tests.The test cases cover all essential validation paths: empty URIs, excessively long URIs, path traversal sequences, spaces, and various valid URI formats including OCI URIs. This thorough approach will help catch validation issues early.
55-65: Well-structured scheme validation tests.You've properly tested both valid schemes (HTTP, HTTPS, Git, SSH, Git+HTTPS, Git+SSH) and invalid schemes (FTP). This ensures the function handles all expected inputs correctly.
68-86: Good coverage of URL scheme enforcement scenarios.This test effectively verifies three key behaviors:
- Preserving existing schemes
- Rewriting SCP-style URLs to use SSH
- Defaulting to HTTPS when no scheme is present
These align well with the PR objective to support SSH and handle SCP URL alignment.
89-103: Solid test for SCP URL rewriting.The test correctly verifies both positive and negative cases for SCP URL rewriting, which is central to the PR's goals of supporting SSH and SCP URL alignment.
194-221: Good platform-aware symlink testing.The test correctly skips symlink tests on Windows where symlink behavior differs, creates a proper test environment, and verifies both the removal of symlinks and preservation of regular files.
223-258: Comprehensive file download test with proper cleanup.This test thoroughly validates the file download functionality by creating temporary directories, writing test content, performing the download, and verifying the result. The Windows skip and proper cleanup with defer are good practices.
338-342: Good test cleanup practice.Properly restoring original detectors in TestMain ensures tests don't interfere with each other or leave the environment in an altered state.
| @@ -13,7 +13,7 @@ comment: | |||
| layout: "reach,diff,flags,tree" # Display different coverage views | |||
| behavior: default # Default PR comment behavior | |||
| require_changes: true # Only post if coverage changes | |||
There was a problem hiding this comment.
| require_changes: true # Only post if coverage changes | |
| require_changes: true # Only post if coverage changes |
| @@ -13,7 +13,7 @@ comment: | |||
| layout: "reach,diff,flags,tree" # Display different coverage views | |||
| behavior: default # Default PR comment behavior | |||
| require_changes: true # Only post if coverage changes | |||
| require_base: yes # Compare against base branch coverage | |||
| require_base: true # Compare against base branch coverage | |||
There was a problem hiding this comment.
| require_base: true # Compare against base branch coverage | |
| require_base: true # Compare against base branch coverage |
| @@ -19,6 +19,9 @@ import ( | |||
| u "github.com/cloudposse/atmos/pkg/utils" | |||
| ) | |||
|
|
|||
| // Dedicated logger for stderr to keep stdout clean of detailed messaging, e.g. for files vendoring. | |||
| var StderrLogger = log.New(os.Stderr) | |||
There was a problem hiding this comment.
We do not need to introduce a new logger. We use log.* everywhere now without this.
| @@ -508,7 +511,7 @@ func generateSkipFunction(tempDir string, s *schema.AtmosVendorSource) func(os.F | |||
| } | |||
|
|
|||
| // If 'included_paths' is not provided, include all files that were not excluded | |||
| log.Debug("Including", u.TrimBasePathFromPath(tempDir+"/", src)) | |||
| StderrLogger.Debug("Including", "path", u.TrimBasePathFromPath(tempDir+"/", src)) | |||
There was a problem hiding this comment.
| StderrLogger.Debug("Including", "path", u.TrimBasePathFromPath(tempDir+"/", src)) | |
| log.Debug("Including", "path", u.TrimBasePathFromPath(tempDir+"/", src)) |
📝 WalkthroughWalkthroughThis pull request updates various configuration files, internal Git URL handling utilities, logging mechanisms, and documentation. It enhances CI/CD workflows with full repository checkouts and increased timeouts, adjusts linter configurations, and standardizes boolean syntax. The internal Git detector and getter now support multiple Git providers with improved scheme, token, and symlink handling. Comprehensive unit tests have been added along with new URL utility functions. Additionally, several YAML configuration files and snapshot tests for vendoring scenarios are introduced, and the documentation is updated to clarify SSH vendoring and new environment variables for Bitbucket and GitLab. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Detector as CustomGitDetector
participant Getter as CustomGitGetter
participant Git as Git Repository
Client->>Detector: Request Git URL transformation
Detector->>Detector: Ensure scheme and normalize path
Detector->>Detector: Inject and resolve token
Detector-->>Client: Return transformed URL
Client->>Getter: Initiate repository clone
Getter->>Git: Execute clone operation
Getter->>Getter: Remove symlinks from clone
Getter-->>Client: Return repository content
Assessment against linked issues
Possibly related PRs
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. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)internal/exec/go_getter_utils.go (2)⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (14)
✨ 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: 1
♻️ Duplicate comments (1)
internal/exec/go_getter_utils.go (1)
211-243:⚠️ Potential issueAlign token fallback with doc statements.
Docs sayATMOS_BITBUCKET_TOKENhas higher precedence thanBITBUCKET_TOKEN, but code does the opposite. Please resolve for consistency.case hostBitbucket: - tokenSource = "BITBUCKET_TOKEN" + tokenSource = "ATMOS_BITBUCKET_TOKEN" token = os.Getenv(tokenSource) if token == "" { - tokenSource = "ATMOS_BITBUCKET_TOKEN" + tokenSource = "BITBUCKET_TOKEN" token = os.Getenv(tokenSource) }
🧹 Nitpick comments (29)
tests/fixtures/scenarios/vendoring-dry-run/atmos.yaml (2)
13-13: Fix trailing spaces to maintain clean codeRemove the trailing spaces at the end of this line to comply with YAML formatting standards.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
1-23: Remove excess blank linesThere are too many blank lines at the beginning and end of the file. Consider removing them for cleaner code style.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 22-22: too many blank lines
(2 > 0) (empty-lines)
tests/fixtures/scenarios/vendor-creds-sanitize/atmos.yaml (2)
1-1: Remove unnecessary blank line at the beginning of the file.Starting the file directly with content improves readability and follows YAML best practices.
- base_path: "./"🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
13-13: Remove trailing whitespace.Remove trailing spaces at the end of line 13 to maintain clean code and avoid linter warnings.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml (1)
20-20: Add newline at end of file.Add a newline character at the end of the file to adhere to standard file formatting conventions and avoid linter warnings.
logs: file: "/dev/stderr" - level: Info + level: Info +🧰 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 (1)
17-17: Fix typos in the comment.There are two spelling errors in this comment line.
- ## Explicit ssh vednoring (the schema is explicitly spcified along with a username, no custom detector is invoked) + ## Explicit ssh vendoring (the schema is explicitly specified along with a username, no custom detector is invoked)tests/fixtures/scenarios/vendor-creds-sanitize/vendor.yaml (2)
29-35: Address formatting issues.The file is missing a newline at the end and has trailing whitespace on line 18.
terraform-null-label.git?ref={{ .Version }}" version: "0.25.0" targets: - "library/cred/{{ .Component }}" tags: - demo +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 35-35: no new line character at the end of file
(new-line-at-end-of-file)
18-18: Remove trailing whitespace.- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: trailing spaces
(trailing-spaces)
website/docs/cli/commands/vendor/vendor-pull.mdx (1)
214-222: URL resolution rules are clearly definedThe documentation clearly outlines the three main rules for how Atmos resolves different URL formats.
Consider using the typographical ellipsis character (…) instead of three periods in line 218:
-1. If a **full HTTPS URL** is provided (`git::https://github.com/...`), it is used as-is. No token data is injected, even if environment variables are set. +1. If a **full HTTPS URL** is provided (`git::https://github.com/…`), it is used as-is. No token data is injected, even if environment variables are set.🧰 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)
tests/test-cases/vendoring-ssh-dryrun.yaml (2)
57-57: Remove trailing spaces.
There are trailing spaces at the end of line 57 that can cause style or linting issues.Apply this fix:
- - !not 'supersecret' + - !not 'supersecret'🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
59-59: Add a newline at the end of file.
A final newline helps maintain consistency and avoids issues with certain editors or tools.Apply this fix:
- exit_code: 0 \ No newline at end of file + exit_code: 0🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
website/docs/cli/configuration/configuration.mdx (5)
681-681: Clarify wording about Bitbucket rate limits.
The phrase "Unauthenticated Requests are limited to 60 requests per hour across all API resources" is capitalized inconsistently and may confuse some users. Consider rephrasing to keep style consistent with other docs.
682-682: Expand usage details for Bitbucket username.
It might help users if you clarify how to set this environment variable and the fallback behavior in more detail, aligning with the note ingo_getter_utils.go.
683-683: Add GitLab usage examples.
Consider including a short example or reference link on how to obtain and use GitLab PATs for first-time users.
699-699: Correct minor typo.
"comamnds" → "commands"- | ATMOS_TERRAFORM_WORKSPACE | The name of the Terraform workspace in which Terraform comamnds should be run | + | ATMOS_TERRAFORM_WORKSPACE | The name of the Terraform workspace in which Terraform commands should be run |
701-701: Explain TF_CLI_ARGS_ usage.*
Users may not understand the significance of these environment variables. A small description or example referencing Terraform’s documentation would be helpful.internal/exec/go_getter_utils.go (10)
15-15: Use consistent import aliases for “github.com/charmbracelet/log.”
Project guidelines appear to recommend importing it with aliaslog. Confirm that the current alias matches the standard.
66-70: Consider making 'source' private.
To ensure encapsulation, you might consider making thesourcefield inCustomGitDetectorprivate or track usage across the codebase.
74-124: Ensure correct handling of unsupported hosts.
If future Git providers must be supported, consider a tertiary fallback or default token approach.
143-161: Encapsulate scheme logic.
The approach to rewriting SCP-style URLs and defaulting to https is correct. Alternatively, consider a single function that deduces scheme to reduce branching.
163-186: Validate rewriteSCPURL assumptions.
Confirm that no scenarios exist where rewriting might break non-standard subpaths or custom domains.Would you like a shell script to locate all references to
rewriteSCPURLand confirm usage patterns?
188-196: Add a test for path normalization errors.
Currently, ifurl.PathUnescapefails, the code defaults to a fallback. You might want to explicitly test that fallback scenario for coverage.
245-265: Refine logging messages for Bitbucket username.
You are logging the username for debugging. Ensure no sensitive info (like tokens) is displayed if the username is personal.
303-311: Check usage of shallow clone depth.
Settingdepth=1by default is fine for new modules, but consider if advanced usage might require history. Provide an override?
343-357: Explore user-configurable symlink removal.
Completely removing symlinks might cause unexpected behavior for certain repos. Offer an opt-out or configuration in Atmos settings if needed.
359-374: Document large file warnings.
TheDownloadDetectFormatAndParseFilemethod might need a note or guard if the file is huge, given you store it in temp.internal/exec/go_getter_utils_test.go (3)
105-116: Good URL path normalization test.The test confirms spaces are properly handled in URL paths. Consider adding additional test cases for other special characters that might need normalization.
func TestNormalizePath(t *testing.T) { detector := &CustomGitDetector{} uObj, err := url.Parse("https://example.com/some%20path") if err != nil { t.Fatalf("Failed to parse URL: %v", err) } detector.normalizePath(uObj) if !strings.Contains(uObj.Path, " ") { t.Errorf("Expected normalized path to contain spaces, got %s", uObj.Path) } + + // Test additional special characters + uObj2, err := url.Parse("https://example.com/path%3Fwith%23special%26chars") + if err != nil { + t.Fatalf("Failed to parse URL: %v", err) + } + detector.normalizePath(uObj2) + if !strings.Contains(uObj2.Path, "?") || !strings.Contains(uObj2.Path, "#") || !strings.Contains(uObj2.Path, "&") { + t.Errorf("Expected normalized path to contain special characters, got %s", uObj2.Path) + } }
300-310: Valuable additional error path testing.These additional tests strengthen the validation by focusing on specific error cases. Consider consolidating with the main ValidateURI test for better organization.
Consider consolidating these tests with the main TestValidateURI function to keep all URI validation tests in one place.
312-319: Good negative test case for SCP URL rewriting.This test specifically verifies that non-SCP URLs won't be incorrectly rewritten. Consider consolidating with the main rewriteSCPURL test for better organization.
Consider consolidating this test with the main TestRewriteSCPURL function to keep all related tests in one place.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.github/workflows/test.yml(3 hunks).golangci.yml(1 hunks)codecov.yml(1 hunks)internal/exec/go_getter_utils.go(4 hunks)internal/exec/go_getter_utils_test.go(1 hunks)internal/exec/vendor_model.go(2 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/fixtures/scenarios/vendoring-dry-run/atmos.yaml(1 hunks)tests/fixtures/scenarios/vendoring-dry-run/components/terraform/ipinfo/component.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_vendor_pull_component_using_SSH.stderr.golden(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_ssh_component.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 (7)
internal/exec/vendor_model.go (1)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh_component.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-18T00:20:29.425Z
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-18T12:26:25.329Z
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_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-18T00:20:29.425Z
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-18T12:26:25.329Z
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_component_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-18T00:20:29.425Z
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-18T12:26:25.329Z
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-18T00:20:29.425Z
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-18T12:26:25.329Z
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#1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
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.
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-03-18T00:20:29.425Z
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.
internal/exec/go_getter_utils.go (2)
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: internal/exec/go_getter_utils.go:74-75
Timestamp: 2025-03-18T00:20:40.297Z
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.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
🧬 Code Definitions (2)
pkg/utils/url_utils_test.go (1)
pkg/utils/url_utils.go (1) (1)
MaskBasicAuth(10:26)
internal/exec/go_getter_utils_test.go (1)
internal/exec/go_getter_utils.go (12) (12)
ValidateURI(24:51)IsValidScheme(54:64)CustomGitDetector(68:71)rewriteSCPURL(163:186)hostGitHub(136:136)getDefaultUsername(246:265)token(213:213)hostGitLab(137:137)hostBitbucket(138:138)GoGetterGet(292:326)DownloadDetectFormatAndParseFile(360:374)RegisterCustomDetectors(282:289)
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/vendoring-dry-run/atmos.yaml
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 22-22: too many blank lines
(2 > 0) (empty-lines)
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)
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-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] 57-57: trailing spaces
(trailing-spaces)
[error] 59-59: no new line character at the end of file
(new-line-at-end-of-file)
🪛 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 (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (56)
codecov.yml (1)
16-16: Good standardization of boolean representation.Changing from
require_base: yestorequire_base: trueensures consistency with line 15 and follows the standard boolean representation in configuration files.internal/exec/vendor_model.go (2)
15-15: Good job fixing the import alias.The log package is now correctly imported with the required alias according to the project's configuration.
337-337: Added helpful debug log for package installation.This log entry provides valuable visibility for package download operations, which aligns with the PR's goal of enhancing the output of dry run mode.
.golangci.yml (1)
173-173: Appropriate adjustment to nestif complexity threshold.Increasing the minimum complexity from 4 to 6 will reduce noise from the linter while still flagging significantly complex nested structures.
.github/workflows/test.yml (3)
49-50: Good CI enhancement with full repository clone.Setting
fetch-depth: 0ensures the entire commit history is available during CI/CD runs, which improves the reliability of tests that might depend on git history.
88-88: Appropriate timeout increase for the test job.Increasing the timeout from 15 to 20 minutes provides more time for tests to complete, reducing spurious failures due to timing constraints.
159-159: Good timeout adjustment for acceptance tests.Doubling the timeout from 10 to 20 minutes for acceptance tests ensures comprehensive testing can complete without arbitrary time limits.
tests/fixtures/scenarios/vendoring-dry-run/atmos.yaml (1)
2-20: Configuration looks goodThe configuration correctly sets up the vendoring-dry-run scenario with appropriate paths and settings. The GitHub token injection is properly enabled which aligns with the PR objectives to support SSH and detailed SCP URL alignment.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
pkg/utils/url_utils.go (2)
8-8: Good use of a named constantUsing a named constant for the masked secret value improves code readability and maintainability.
10-26: Well-structured URL credential masking implementationThe function appropriately handles different credential scenarios (username+password and username-only) and provides clear error handling. The implementation is clean and follows Go best practices.
tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden (1)
1-10: Test snapshot correctly demonstrates the dry run log outputThe snapshot captures the expected log output for the SSH vendor pull command in dry run mode, showing detailed information about the URL conversion process as described in the PR objectives.
internal/exec/yaml_func_include.go (1)
50-50:❓ Verification inconclusive
Changed parameter passing from value to pointer
Changed the function call to pass a pointer to
atmosConfigrather than the value. This change allows the function to potentially modify the originalatmosConfigobject.Verify that the function signature has been updated to handle this pointer, and that this change is intentional and consistent with other uses of the function.
🏁 Script executed:
#!/bin/bash # Check if the DownloadDetectFormatAndParseFile function now accepts a pointer rg -p "func DownloadDetectFormatAndParseFile" --include "*.go" -A 5Length of output: 511
Below is a revised review comment that reflects the intended pointer change but requests manual confirmation of the updated function signature:
Action Required: Verify Function Signature Update
The code now calls the function with a pointer:
res, err = DownloadDetectFormatAndParseFile(&atmosConfig, f)This change is intended to allow the function to modify the original
atmosConfigobject by passing its address rather than a copy. However, our initial automated search using the--includeflag produced an error. Please manually verify that the function signature forDownloadDetectFormatAndParseFilehas been updated to accept a pointer (e.g.,*AtmosConfig) instead of a value.For example, you can run the following command to check the signature:
rg "func DownloadDetectFormatAndParseFile" -A 5
- Confirm that the parameter for
atmosConfigin the function definition is declared as a pointer.- Ensure this pointer usage is consistent throughout the codebase.
Once confirmed, no further action is needed. If discrepancies arise, please update the function definition accordingly.
tests/fixtures/scenarios/vendor-creds-sanitize/atmos.yaml (1)
2-20: Configuration looks good and properly structured.The Atmos configuration is well-structured with appropriate settings for testing the vendor credential sanitization scenario. The
inject_github_token: truesetting is correctly positioned to test the token injection feature.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
tests/fixtures/scenarios/vendor-pulls-ssh/atmos.yaml (1)
1-19: Configuration looks good.The configuration is well-structured for testing SSH-based vendor pulls. The logging configuration to
/dev/stderrwith Info level aligns well with the PR's goal of enhancing dry run outputs.tests/fixtures/scenarios/vendor-pulls-ssh/vendor.yaml (1)
1-24: SSH vendor configuration looks good.The vendor configuration properly defines both implicit (SCP-style) and explicit SSH vendoring scenarios, which aligns with the PR objective of supporting SSH + detailed SCP URLs. The component definitions include appropriate sources, versions, targets, and tags.
tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh_component.stderr.golden (1)
1-18: Test snapshot correctly captures enhanced dry run output.This test snapshot effectively demonstrates the improved dry run output for SSH URLs, showing:
- SCP-style URL rewriting (line 9)
- Token injection from environment variables (line 11)
- Final sanitized URL format (line 12)
- Clear summary of the dry run completion (line 14)
This aligns perfectly with the PR objective of enhancing the dry run output to display detailed information about SCP-style link conversion, token injection, and URL sanitization.
internal/exec/vendor_utils.go (2)
22-23: Dedicated logger for stderr looks good.The dedicated logger ensures stdout remains clean while detailed messages are sent to stderr, which aligns with the PR objective of enhancing output during dry runs.
514-514: Logger implementation is consistent with requested changes.The updated logging implementation correctly uses the new StderrLogger and follows the suggested key-value format for better readability.
tests/fixtures/scenarios/vendoring-dry-run/components/terraform/ipinfo/component.yaml (1)
7-9: Good use of SSH-style Git URL pattern for the dry run test.The use of SSH-style Git URL in the form
git::[email protected]:cloudposse/atmos.git//examples/...aligns perfectly with the PR objective of supporting SSH URLs in dry run mode. This test fixture will be valuable for validating that the dry run correctly shows information about SSH URLs.tests/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden (1)
8-12: Well implemented credential sanitization and URL transformation logging.The debug logs effectively capture the sequence of URL transformations while properly masking sensitive token information with "xxx:xxx". This implementation fulfills the PR objective of enhancing dry run output to show detailed information about URL transformation without leaking credentials.
tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden (1)
11-13:❓ Verification inconclusive
Verify the apparent contradiction in dry run messages.
There appears to be a contradiction between "Dry run completed. No components vendored" (line 11) and "Vendored 2 components" (line 13). While this might be intentional to show both the dry run message and what would happen in an actual run, it could be confusing to users.
🏁 Script executed:
#!/bin/bash # Check if this pattern appears in other test snapshots rg -A 1 "Dry run completed. No components vendored" tests/snapshots/Length of output: 907
Snapshot Contradiction in Dry Run Message
It looks like in
tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden(lines 11–13) we’re seeing two contrasting messages—one stating "Dry run completed. No components vendored." and another indicating "Vendored 2 components." While similar snapshots consistently include the dry run message, this file also outputs a real run message. Please verify whether this dual output is intentional (to simulate both the dry run and actual execution) or if it’s an oversight that might confuse users. If intentional, adding a clarifying comment in the snapshot could help; if not, consider updating the snapshot to remove the contradiction.tests/fixtures/scenarios/vendor-creds-sanitize/vendor.yaml (2)
11-17: Good test fixture for token injection workflow.The basic configuration correctly demonstrates the default case where a GitHub token should be injected when no credentials are present in the URL. This provides good test coverage for the token injection functionality.
20-26: Good test case for URL with embedded credentials.This configuration properly tests the case where credentials already exist in the URL and token injection should be skipped. Including "supersecret" in this test fixture is appropriate since it's for testing credential handling.
pkg/utils/url_utils_test.go (4)
7-17: Tests for URL credential masking look goodThe test effectively verifies URL masking with both username and password credentials.
19-29: Username-only masking test is properly implementedThis test case correctly confirms that URLs with only a username (no password) are properly masked.
31-41: Non-credential URL test is appropriateTest confirms that URLs without any credentials remain unchanged after processing.
43-49: Invalid URL test is well-formedThe test properly verifies error handling for malformed URLs.
tests/snapshots/TestCLICommands_atmos_vendor_pull_custom_detector_credentials_leakage.stderr.golden (3)
7-11: Git detection and URL handling looks correctThe debug output properly shows the detection process, URL scheme defaulting, and token injection.
12-86: File inclusion logs are consistentDebug statements correctly show all files being included during the vendoring process.
87-92: Component vendoring completion messages are clearThe output indicates successful vendoring of all three components with their versions, making it clear what was processed.
website/docs/cli/commands/vendor/vendor-pull.mdx (3)
111-159: SSH vendoring documentation is thorough and clearThe added documentation for SSH vendoring provides comprehensive guidance on both SCP-style and explicit SSH sources, with clear examples and important limitations noted.
160-173: Git over HTTPS vendoring explanation is conciseThe documentation clearly explains how HTTPS URLs are automatically resolved and transformed.
174-222: Authentication documentation is well-structuredThe section on authentication and token usage provides detailed information about environment variable priorities for different Git providers using appropriate definition lists.
🧰 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)
tests/snapshots/TestCLICommands_atmos_vendor_pull_component_using_SSH.stderr.golden (1)
1-12: Snapshot output matches expected test behaviorThe output captures the expected debug messages and information for a component vendoring operation using SSH. The sequence from dry run completion to final vendoring count is correct for a test snapshot.
website/docs/cli/configuration/configuration.mdx (1)
697-697: Confirm usage of ATMOS_SHLVL across shells.
Ensure the user is aware that some shells might override or ignore custom environment variables. A quick mention can help avoid confusion.internal/exec/go_getter_utils.go (5)
11-11: Verify necessity of new regexp import.
Make sure it is not introducing unnecessary overhead or conflicting with existing patterns.
126-133: Check for potential collisions with constants.
Ensure the named constants for regex captures do not conflict with any other usage in the repo.
198-209: Ensure token usage is logged at appropriate levels.
This logs the presence of a token. Double-check that you do not inadvertently leak partial tokens in production logs.
267-278: Confirm subdirectory logic for single-file repos.
If we are dealing with a single file at the root, appending "//." might cause issues. Confirm usage with single-file repos or set explicit checks.
333-341: Ensure consistency with go-getter’s default approach.
Review whether file permissions or ownership transformations are needed if go-getter performed them by default.internal/exec/go_getter_utils_test.go (16)
1-17: Solid test file setup and imports.The file organization is clean with proper imports needed for testing the go-getter utilities.
18-26: Good approach saving original detectors and creating test helper.Saving
originalDetectorsfor restoration ensures tests don't interfere with each other. ThefakeAtmosConfighelper function simplifies test setup.
28-52: Comprehensive URI validation test cases.The test covers all edge cases: empty URIs, lengthy URIs, path traversal, spaces, and valid formats. Good job testing both error and success paths.
54-65: Well-structured scheme validation tests.Tests verify all supported schemes are accepted and invalid ones rejected. The usage of a string slice for valid schemes makes the test maintainable.
67-86: Thorough scheme enforcement testing.The test properly verifies three key behaviors of
ensureScheme:
- Preserving existing schemes
- Rewriting SCP-style URLs to SSH
- Adding default HTTPS scheme when missing
88-103: Clear SCP URL rewriting tests.Test validates both the positive case (SCP URL is rewritten) and negative case (non-SCP URL remains unchanged). This ensures the function behaves correctly in all scenarios.
118-137: Effective token injection test.Test properly sets up the environment variable, tests the injection, and cleans up afterward. The validation of the username is thorough.
139-152: Well-structured token resolution test.The test verifies both the token value and its source, ensuring complete validation of the function's behavior.
154-170: Comprehensive username handling test across platforms.Test covers all supported Git hosting services and unknown hosts. Good job testing environment variable overrides for Bitbucket.
172-192: Clear subdirectory adjustment tests.Tests both cases: adding "/." to paths without subdirectories and preserving paths with subdirectories. The assertions validate the exact behavior needed.
194-221: Thorough symlink removal test with platform awareness.Good platform check to skip on Windows. The test creates actual files and symlinks, verifying removal works correctly. Proper cleanup with defer ensures no test artifacts remain.
223-258: Functional end-to-end file transfer test.Test validates actual file content transfer, which is important for ensuring the function works in real conditions. Good platform check for Windows compatibility.
260-283: Effective format detection and parsing test.Test creates and parses actual JSON content, validating the full workflow. Good type assertion and content verification.
285-298: Proper detector registration test.Test isolates the detector registry by saving and restoring the original state. Good type assertion to verify the correct detector is registered first.
321-336: Thorough error path testing for path normalization.The test directly constructs a URL with invalid escaping to test error handling, which is a good approach for error path testing. The assertions verify the function doesn't modify invalid paths.
338-342: Clean test cleanup in TestMain.Using TestMain to restore the original detectors ensures the test suite doesn't affect other tests. This is excellent test hygiene.
| } | ||
|
|
||
| // RegisterCustomDetectors prepends the custom detector so it runs before | ||
| // the built-in ones. Any code that calls go-getter should invoke this. | ||
| func RegisterCustomDetectors(atmosConfig schema.AtmosConfiguration) { | ||
| func RegisterCustomDetectors(atmosConfig *schema.AtmosConfiguration) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid global state mutations in RegisterCustomDetectors.
Registering the custom detector modifies a global variable in go-getter. Ensure concurrency safety if the code can run in parallel.
Listener430
force-pushed
the
DEV-2964_vendoring-dry-run
branch
from
9dd664e to
f222283
Compare
what
This PR upgrades the output of dry run mode for vendor pull command.
After the change is applied, the dry run mode shows
yet there's no actual files download.
Before ( component vendoring dry-run output)
After ( component vendoring dry-run output)
component.yaml, refer to the respective test case for details
Before ( generic vendoring dry-run output)
After ( generic vendoring dry-run output)
vendor.yaml, refer to the respective test case for detaails
why
Received feedback from in the parent PR
references
Upscopes from #1061 and
Summary by CodeRabbit
New Features
Documentation