Fix vendor pull directory creation issue #782
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (1)
internal/exec/vendor_utils.go (1)
554-557: LGTM! This is a solid fix for the directory creation issue.The addition of
os.MkdirAllbefore the copy operation is exactly what was needed to resolve the "no such file or directory" error. The error handling is well-structured and provides clear feedback.A small suggestion to make the code even more robust:
targetDir := filepath.Dir(targetPath) -if err := os.MkdirAll(targetDir, 0755); err != nil { +if err := os.MkdirAll(targetDir, 0o755); err != nil { return fmt.Errorf("failed to create target directory '%s': %w", targetDir, err) }Using
0o755instead of0755makes the octal permission more explicit and follows modern Go conventions.
|
@Cerebrovinny I do not think this is the fix based on the description of the problem. Bear in mind we have two types of vendor manifests. https://atmos.tools/core-concepts/vendor/ The bug report concerns the component manifest file. That means that any logic relating to the vendor manifests file should not inhibit the component manifest from functioning. To me, it sounds like the regression is that when vendoring a component manifest, it is erroring if the vendor manifests file path does not exist. However, that should not be required. We should instead log to warning that the path does not exist and continue processing. We should not create the vendor configuration path if it does not exist. |
| @@ -83,7 +83,7 @@ func ExecuteVendorPullCommand(cmd *cobra.Command, args []string) error { | |||
|
|
|||
| // Check `vendor.yaml` | |||
| vendorConfig, vendorConfigExists, foundVendorConfigFile, err := ReadAndProcessVendorConfigFile(cliConfig, cfg.AtmosVendorConfigFileName) | |||
| if err != nil { | |||
| if vendorConfigExists && err != nil { | |||
There was a problem hiding this comment.
I think we should log a warning to make it easier to debug
There was a problem hiding this comment.
@coderabbitai any suggestions?
There was a problem hiding this comment.
@osterman, we can add a warning log when the vendor config file does not exist to make debugging easier. This way, we inform the user and allow the process to continue.
🧠 Learnings used
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.
There was a problem hiding this comment.
@coderabbitai please make the suggestion
There was a problem hiding this comment.
@osterman, to implement the warning log, we can modify the ReadAndProcessVendorConfigFile function to log a warning when the vendor config file does not exist and allow the process to continue. Here's how we can adjust the code:
In ReadAndProcessVendorConfigFile, after attempting to locate the vendor config file, check if the file does not exist, log a warning, set vendorConfigFileExists to false, and return without an error:
// After attempting to find the vendor config file
if !vendorConfigFileExists {
u.LogWarning(cliConfig, fmt.Sprintf("Vendor config file '%s' does not exist. Proceeding without vendor configurations.", vendorConfigFile))
return vendorConfig, vendorConfigFileExists, "", nil
}In the calling function ExecuteVendorPullCommand, adjust the error handling condition to remove the check for vendorConfigExists, since the absence of the file is no longer an error:
- if vendorConfigExists && err != nil {
+ if err != nil {
return err
}This way, if the vendor config file is not present, a warning will be logged, and the process will continue without interruption.
Let me know if this approach works for you.
🧠 Learnings used
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.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
what
Fixed the regression where the vendor command would error if the vendor manifests file path did not exist.
why
Previously, the atmos vendor pull command would fail with a “no such file or directory” error when the vendor manifests file path was missing.
references
Closes #780
Logic design
Summary by CodeRabbit