Add support for vendor path setting in atmos.yaml
#737
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request introduce a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- atmos.yaml (1 hunks)
- internal/exec/vendor_utils.go (1 hunks)
- pkg/schema/schema.go (2 hunks)
🧰 Additional context used
🪛 yamllint
atmos.yaml
[error] 359-359: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
pkg/schema/schema.go (2)
18-18: LGTM: Addition of Vendor field to CliConfigurationThe new
Vendorfield in theCliConfigurationstruct is well-implemented. It follows the existing naming conventions and properly uses theomitemptytag, making it an optional configuration. This change aligns with the PR objective of supporting vendor path specification in the Atmos YAML configuration.
18-18: Summary: Vendor configuration support successfully implementedThe changes made to
pkg/schema/schema.goeffectively implement support for vendor path specification in the Atmos YAML configuration. The additions are well-integrated into the existing code structure and follow established conventions. These modifications enable users to define vendor settings, aligning with the PR's objectives.Key points:
- The
CliConfigurationstruct now includes an optionalVendorfield.- A new
Vendorstruct has been added to represent vendor-specific settings.These changes provide a solid foundation for handling vendor configurations in the Atmos setup.
Also applies to: 549-551
osterman
changed the title
Cerebrovinny
force-pushed
the
feat/dev-2378
branch
from
cca19f7 to
626c3e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- atmos.yaml (1 hunks)
- internal/exec/vendor_utils.go (1 hunks)
- pkg/schema/schema.go (2 hunks)
🧰 Additional context used
🪛 yamllint
atmos.yaml
[error] 359-359: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (7)
atmos.yaml (1)
355-359: Excellent addition of the vendor configuration!The new
vendorsection with thevendor_yaml_pathsetting is well-implemented and aligns perfectly with the PR objectives. Great job on:
- Providing a meaningful default path ("./vendor.yaml").
- Supporting both absolute and relative paths.
- Allowing override via environment variable or command-line argument.
This implementation offers flexibility and clear configuration options for users.
🧰 Tools
🪛 yamllint
[error] 359-359: no new line character at the end of file
(new-line-at-end-of-file)
pkg/schema/schema.go (1)
18-18: LGTM: CliConfiguration struct updated correctlyThe addition of the
Vendorfield to theCliConfigurationstruct is well-implemented. The use ofomitemptyin the tags is appropriate, making the field optional in serialized forms. This change aligns with the PR objective of introducing support for specifying a vendor path within the Atmos YAML configuration file.internal/exec/vendor_utils.go (5)
137-143: Well-implemented handling of custom vendor YAML pathThe logic correctly checks if a
vendor_yaml_pathis specified in theatmos.yamlconfiguration and appropriately constructs an absolute path if necessary. It enhances flexibility for users to define custom paths.
145-149: Effective addition of default paths to checkBy including both the current directory and the
base_pathdirectory inpathsToCheck, the code ensures that the vendor configuration file can be located even if it's placed in different common directories.
152-158: Efficient search for the vendor configuration fileThe loop over
pathsToCheckefficiently finds the first existing vendor config file without unnecessary checks, improving the performance of the configuration loading process.
160-162: Clear error messaging when vendor config file is not foundReturning an error with the list of paths tried provides valuable debugging information to the user, aiding in quick resolution of configuration issues.
177-180: Proper validation of the vendor config 'kind' fieldThe code correctly validates the
kindfield of the vendor configuration, ensuring that only supported configurations are processed, which enhances robustness.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- atmos.yaml (1 hunks)
- internal/exec/vendor_utils.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
internal/exec/vendor_utils.go (4)
Line range hint
83-89: LGTM!The error handling and vendorConfig existence check are properly implemented.
142-155: LGTM with existing suggestions!The fallback logic for finding the vendor config file is correct, but please address the existing comment about using
filepath.Joininstead ofpath.Join.
159-159: LGTM!The file reading and validation logic is well-implemented with proper error handling.
131-141: Verify usage of vendor yaml path in the codebaseLet's verify how the new vendor yaml path feature is used across the codebase to ensure compatibility.
✅ Verification successful
Let me gather more context about the implementation to ensure we fully understand the impact.
Let me try one more verification to check the complete implementation context:
Based on the gathered information, I can now provide a final response:
The vendor yaml path changes are safely implemented
The code changes are safe and well-integrated for the following reasons:
- The
VendorYamlPathis properly defined in theVendorstruct inpkg/schema/schema.gowith appropriate yaml/json/mapstructure tags- The implementation in
vendor_utils.gohandles both absolute and relative paths correctly- The feature is only used in
ReadAndProcessVendorConfigFilewhich is called in just two places, both invendor_utils.go- The changes maintain backward compatibility as the
VendorYamlPathcheck is guarded by an empty string check🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to vendor yaml configuration to verify compatibility # with the new vendor yaml path feature. # Test 1: Find all references to vendor config file echo "=== Finding references to vendor config file ===" rg -l "vendor\.ya?ml" # Test 2: Find all calls to ReadAndProcessVendorConfigFile echo -e "\n=== Finding calls to ReadAndProcessVendorConfigFile ===" ast-grep --pattern 'ReadAndProcessVendorConfigFile($$$)' # Test 3: Find all references to VendorYamlPath in struct initialization echo -e "\n=== Finding VendorYamlPath usage ===" rg "VendorYamlPath"Length of output: 1757
Script:
#!/bin/bash # Let's examine the ReadAndProcessVendorConfigFile implementation and its usage context # Check the implementation of ReadAndProcessVendorConfigFile echo "=== ReadAndProcessVendorConfigFile implementation ===" ast-grep --pattern 'func ReadAndProcessVendorConfigFile($$$) { $$$ }' # Check the Vendor struct definition and usage echo -e "\n=== Vendor struct definition ===" ast-grep --pattern 'type Vendor struct { $$$ }' # Look for any tests related to vendor yaml path echo -e "\n=== Tests related to vendor yaml path ===" rg -l "VendorYamlPath.*Test"Length of output: 656
Script:
#!/bin/bash # Let's examine the complete implementation context # Check the full content of vendor_utils.go to understand the implementation echo "=== Vendor utils implementation context ===" rg -A 10 -B 10 "func ReadAndProcessVendorConfigFile" internal/exec/vendor_utils.go # Check for any error handling or validation related to vendor yaml path echo -e "\n=== Error handling related to vendor yaml path ===" rg -A 5 -B 5 "VendorYamlPath.*err" # Check for any config validation echo -e "\n=== Config validation ===" rg -A 5 -B 5 "func.*[Vv]alidate.*Vendor"Length of output: 1132
atmos.yaml (1)
355-359: LGTM! Well-structured vendor configuration.The new vendor configuration follows the established pattern of other path configurations in the file, with:
- Clear documentation of supported path types (absolute/relative)
- Consistent environment variable override support
- Command-line argument override support
There was a problem hiding this comment.
@Cerebrovinny the PR looks good, please address the comment
aknysh
added
no-release
aknysh
removed
the
no-release
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
internal/exec/vendor_utils.go (3)
126-166: Consider improving error handling for base path resolutionThe logic for resolving vendor config paths is correct, but the error handling could be more informative.
Consider adding more context to error messages when paths can't be resolved:
- return vendorConfig, vendorConfigFileExists, "", fmt.Errorf("vendor config file or directory '%s' does not exist", pathToVendorConfig) + return vendorConfig, vendorConfigFileExists, "", fmt.Errorf("vendor config file or directory '%s' does not exist (base_path: %s)", pathToVendorConfig, cliConfig.BasePath)
187-228: Consider adding validation for source component namesWhile the code handles duplicate components well, it might be worth adding validation for component name format.
Consider adding validation to ensure component names follow a consistent format:
+ // Validate component name format + if source.Component != "" { + if !isValidComponentName(source.Component) { + return vendorConfig, false, "", fmt.Errorf("invalid component name '%s' in config file '%s'. Component names must match pattern [a-z0-9-]+", + source.Component, configFile) + } + } if sourceMap[source.Component] {
192-223: Consider adding logging for skipped duplicate importsThe code silently skips duplicate imports. While this is correct behavior, adding debug logs would help with troubleshooting.
Consider adding debug logging for skipped imports:
if importMap[imp] { + u.LogDebug(cliConfig, fmt.Sprintf("Skipping duplicate import '%s' in config file '%s'", imp, configFile)) continue // Skip duplicate imports }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/vendor_utils.go(5 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/vendor_utils.go (2)
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:131-141
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the `ReadAndProcessVendorConfigFile` function in `internal/exec/vendor_utils.go`, the existence of the vendor config file is already checked, so additional file existence checks may be unnecessary.
🔇 Additional comments (3)
internal/exec/vendor_utils.go (3)
9-9: LGTM: Import changes are well-structured
The addition of sort, upgrade to yaml.v3, and inclusion of doublestar/v4 are appropriate for the new functionality.
Also applies to: 18-18, 20-20
169-182: LGTM: Robust directory handling with doublestar
The implementation correctly handles both single file and directory-based configurations, with proper sorting and empty directory checks.
600-600: LGTM: Proper file tracking in imported sources
The code correctly tracks the origin file for each source in imported configurations.
aknysh
changed the title
atmos.yaml
|
These changes were released in v1.106.0. |
what
Added new support for vendor files under folders or multiple vendor files to be processed in Lexicographic order
Refactored logic that does check the presence of vendor file
why
references
#DEV-2378
New tests after general refactoring base path
Summary by CodeRabbit
Release Notes
New Features
vendorsection in theatmos.yamlconfiguration, allowing users to specify abase_pathfor vendor files.--vendor-base-pathto enhance configuration options.Configuration Updates
Documentation