Add option to exclude parent directory when using * wildcard for scp-action #198
Conversation
|
""" WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ForestHouse2316, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a significant enhancement to how scp-action handles file transfers, particularly when dealing with wildcards. The core problem addressed is the unintended copying of parent directories (e.g., dist/) when a user only wants the contents of that directory to be transferred. My changes introduce logic to detect common parent directories among source files and then adjust the tar command to effectively 'cd' into that parent directory before archiving, ensuring only the desired files and their relative paths are included. This prevents the creation of redundant nested directories on the target system, streamlining the deployment process for users who rely on wildcard patterns.
Highlights
- Intelligent Parent Directory Detection: I've modified the
buildTarArgsfunction to intelligently detect if all source files provided share a common parent directory. This is crucial for scenarios where users intend to copy only the contents of a directory (e.g.,dist/*) rather than the directory itself. - Excluding Parent Directory in
tarArchives: When a common parent directory is found, I've adjusted thetarcommand arguments. Specifically, I'm now using the-Coption to change the directory to the common parent before adding the files. This, combined with converting file paths to be relative to this new base, ensures that the parent directory is not included in the archive, resolving the issue of unwanted nested folders on the remote server. - Support for Excluded Paths (
!) with Relative Paths: The new logic also correctly handles paths that are prefixed with!, which are typically used for exclusion. These paths are made relative to the common parent while retaining their!prefix, ensuring that exclusion rules are respected even with the new directory handling.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to strip the common parent directory from source paths, which is useful when using wildcards like dist/*. The implementation correctly identifies the common base path. However, there are a few key areas for improvement. Most importantly, the feature is currently always on, which is a breaking change. As you mentioned in the PR description, this should be behind a feature flag. Additionally, the error handling could be more robust, and the code could be more idiomatic in its path manipulation. Finally, new unit tests are needed to cover this new functionality and prevent future regressions.
| // For precise operation, adding an additional on/off option needed. | ||
| // e.g. SCP_ACTION_WILDCARD_COMPATIBLE | ||
| hasCommonFolder := true | ||
| var basePrefix string | ||
| if len(files.Source) > 0 { | ||
| basePrefix = strings.TrimPrefix(filepath.Dir(files.Source[0]), "!") | ||
| for i := 1; i < len(files.Source) && hasCommonFolder; i++ { | ||
| comparePath := files.Source[i] | ||
| if strings.HasPrefix(files.Source[i], "!") { | ||
| comparePath = comparePath[1:] | ||
| } | ||
| for !strings.HasPrefix(comparePath, basePrefix) { | ||
| lastSlashIdx := strings.LastIndex(basePrefix, string(os.PathSeparator)) | ||
| if lastSlashIdx == -1 { | ||
| hasCommonFolder = false // if Source[i] doesn't have same prefix | ||
| break | ||
| } | ||
| basePrefix = basePrefix[:lastSlashIdx] // shrink prefix range | ||
| } | ||
| } | ||
| } else { | ||
| hasCommonFolder = false | ||
| } | ||
| if hasCommonFolder { // if all files are in basePrefix folder, change execution position | ||
| args = append(args, "-C", basePrefix) | ||
| var relativePaths []string | ||
| for _, path := range files.Source { | ||
| ignorePrefix := "" | ||
| if strings.HasPrefix(path, "!") { | ||
| path = path[1:] | ||
| ignorePrefix = "!" | ||
| } | ||
| rel, err := filepath.Rel(basePrefix, path) | ||
| if err != nil { | ||
| fmt.Printf("Error while processing relative paths") | ||
| continue | ||
| } | ||
| relativePaths = append(relativePaths, ignorePrefix + rel) | ||
| } | ||
| args = append(args, relativePaths...) // modified argument appending | ||
| } else { | ||
| args = append(args, files.Source...) // original argument appending | ||
| } |
There was a problem hiding this comment.
This is a great feature, but as you've noted in the PR description, there are a couple of critical points to address before this can be merged:
- Feature Flag: The new logic is always active. This should be controlled by a configuration option (e.g.,
SCP_ACTION_WILDCARD_COMPATIBLE) to avoid breaking existing workflows for users who expect the original behavior. - Unit Tests: The change lacks unit tests for the new path stripping logic. New tests are needed to cover various scenarios (e.g., common parent, no common parent, mixed ignored/non-ignored paths) and ensure the feature works as expected. These tests should also be conditional on the new feature flag.
plugin.go
Outdated
| hasCommonFolder = false // if Source[i] doesn't have same prefix | ||
| break | ||
| } | ||
| basePrefix = basePrefix[:lastSlashIdx] // shrink prefix range |
There was a problem hiding this comment.
Using filepath.Dir() is more idiomatic and robust for getting the parent directory of a path than manually slicing the string with strings.LastIndex. It correctly handles various edge cases and improves cross-platform compatibility.
| basePrefix = basePrefix[:lastSlashIdx] // shrink prefix range | |
| basePrefix = filepath.Dir(basePrefix) // shrink prefix range |
| } | ||
| rel, err := filepath.Rel(basePrefix, path) | ||
| if err != nil { | ||
| fmt.Printf("Error while processing relative paths") |
There was a problem hiding this comment.
Errors from filepath.Rel are printed to standard output with a generic message. This can be easily missed. It's better to log errors to standard error (os.Stderr) and include the paths that caused the error for easier debugging.
| fmt.Printf("Error while processing relative paths") | |
| fmt.Fprintf(os.Stderr, "Error while processing relative path for '%s' from base '%s': %v\n", path, basePrefix, err) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // For precise operation, adding an additional on/off option needed. | ||
| // e.g. SCP_ACTION_WILDCARD_COMPATIBLE | ||
| hasCommonFolder := true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the wildcard compatibility feature optional to avoid breaking changes.
The comments indicate this feature should have an on/off option, but the current implementation makes it mandatory. This could break existing workflows that depend on preserving the directory structure.
Add a configuration field to make this feature optional:
- Add a field to the
Configstruct:
WildcardCompatible bool- Wrap the new logic in a conditional:
- // For precise operation, adding an additional on/off option needed.
- // e.g. SCP_ACTION_WILDCARD_COMPATIBLE
- hasCommonFolder := true
+ if !p.Config.WildcardCompatible {
+ args = append(args, files.Source...)
+ return args
+ }
+
+ hasCommonFolder := trueThis ensures backward compatibility while providing the new functionality when explicitly enabled.
🤖 Prompt for AI Agents
In plugin.go around lines 188 to 190, the wildcard compatibility feature is
currently always enabled, which risks breaking existing workflows. To fix this,
add a boolean field named WildcardCompatible to the Config struct to control
this feature. Then, wrap the logic that sets hasCommonFolder or related wildcard
compatibility code inside a conditional that checks this new field, enabling the
feature only when explicitly set to true. This preserves backward compatibility
while allowing the new behavior when desired.
| if hasCommonFolder { // if all files are in basePrefix folder, change execution position | ||
| args = append(args, "-C", basePrefix) | ||
| var relativePaths []string | ||
| for _, path := range files.Source { | ||
| ignorePrefix := "" | ||
| if strings.HasPrefix(path, "!") { | ||
| path = path[1:] | ||
| ignorePrefix = "!" | ||
| } | ||
| rel, err := filepath.Rel(basePrefix, path) | ||
| if err != nil { | ||
| fmt.Printf("Error while processing relative paths") | ||
| continue | ||
| } | ||
| relativePaths = append(relativePaths, ignorePrefix + rel) | ||
| } | ||
| args = append(args, relativePaths...) // modified argument appending | ||
| } else { | ||
| args = append(args, files.Source...) // original argument appending | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add validation for relative path conversion.
The current error handling is insufficient and could lead to inconsistent tar archives.
if hasCommonFolder { // if all files are in basePrefix folder, change execution position
+ // Validate that basePrefix is a valid directory
+ if info, err := os.Stat(basePrefix); err != nil || !info.IsDir() {
+ // Fallback to original behavior if basePrefix is not a valid directory
+ args = append(args, files.Source...)
+ return args
+ }
+
args = append(args, "-C", basePrefix)
var relativePaths []string
for _, path := range files.Source {
ignorePrefix := ""
if strings.HasPrefix(path, "!") {
path = path[1:]
ignorePrefix = "!"
}
rel, err := filepath.Rel(basePrefix, path)
if err != nil {
- fmt.Printf("Error while processing relative paths")
+ fmt.Printf("Error converting path %q to relative path from %q: %v\n", path, basePrefix, err)
+ // Fallback to original behavior on any error
+ args = append(args, files.Source...)
+ return args
}
relativePaths = append(relativePaths, ignorePrefix + rel)
}
args = append(args, relativePaths...) // modified argument appending
} else {
args = append(args, files.Source...) // original argument appending
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hasCommonFolder { // if all files are in basePrefix folder, change execution position | |
| args = append(args, "-C", basePrefix) | |
| var relativePaths []string | |
| for _, path := range files.Source { | |
| ignorePrefix := "" | |
| if strings.HasPrefix(path, "!") { | |
| path = path[1:] | |
| ignorePrefix = "!" | |
| } | |
| rel, err := filepath.Rel(basePrefix, path) | |
| if err != nil { | |
| fmt.Printf("Error while processing relative paths") | |
| continue | |
| } | |
| relativePaths = append(relativePaths, ignorePrefix + rel) | |
| } | |
| args = append(args, relativePaths...) // modified argument appending | |
| } else { | |
| args = append(args, files.Source...) // original argument appending | |
| } | |
| if hasCommonFolder { // if all files are in basePrefix folder, change execution position | |
| // Validate that basePrefix is a valid directory | |
| if info, err := os.Stat(basePrefix); err != nil || !info.IsDir() { | |
| // Fallback to original behavior if basePrefix is not a valid directory | |
| args = append(args, files.Source...) | |
| return args | |
| } | |
| args = append(args, "-C", basePrefix) | |
| var relativePaths []string | |
| for _, path := range files.Source { | |
| ignorePrefix := "" | |
| if strings.HasPrefix(path, "!") { | |
| path = path[1:] | |
| ignorePrefix = "!" | |
| } | |
| rel, err := filepath.Rel(basePrefix, path) | |
| if err != nil { | |
| fmt.Printf("Error converting path %q to relative path from %q: %v\n", path, basePrefix, err) | |
| // Fallback to original behavior on any error | |
| args = append(args, files.Source...) | |
| return args | |
| } | |
| relativePaths = append(relativePaths, ignorePrefix+rel) | |
| } | |
| args = append(args, relativePaths...) // modified argument appending | |
| } else { | |
| args = append(args, files.Source...) // original argument appending | |
| } |
🤖 Prompt for AI Agents
In plugin.go around lines 211 to 230, the error handling when filepath.Rel fails
is insufficient and may cause inconsistent tar archives. Improve this by logging
the error details clearly instead of just printing a generic message, and
consider returning or handling the error to prevent appending invalid relative
paths. Add validation to ensure relative paths are correctly computed before
appending them to args, and handle any errors gracefully to maintain consistent
archive creation.
Possible issue by AI review said my code can occur an error when input is [ test1/a.txt , test12/b.txt ] I fixed that part
1 participant
Modification explanation
Using scp-action, I found that this action also copy the parent(top) folder of the source when I wrote source like
dist/*.And the result is:
This image shows that the scp-action has copied file to target folder of my remote server, but it is in the
distfolder.I thought it'll be nice to add an option for the situation setting source files with one folder and asterisk wildcard to drone-scp.
This PR implemented it.
(The dist folder still exist because I didn't removed it. This code just works well 👍)
This time, I used modified scp-action which downloads forked version of drone-scp.
With same source and target setting, scp-action only copied and overwritten the files IN the
distfolder.Additional example
Points for improvements
Serve this function as an option
Because this function is made from needs about scp-action, it might not so useful when using only drone-scp.
So I think this function should be able to be turned off by the option.
Unit test fail
Current unittest system is working on the premise that drone-scp preserves all folder structure when input is
[ top/a.txt , top/b.txt ].But this function doesn't preserve the folder
topin this case.Therefore, at first, function on/off option is needed.
And making a new unit test for the function turned on situation is needed.
Summary by CodeRabbit