-
Notifications
You must be signed in to change notification settings - Fork 28
feat(create): add '--subdir <path>' flag to 'create' command #345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e6cf706
62d6b43
e477a49
b1312a7
a62cbcf
78ccb06
03222ca
32ede49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -45,11 +45,18 @@ import ( | |||||
| "github.com/spf13/afero" | ||||||
| ) | ||||||
|
|
||||||
| // copyIgnoreDirectories are directories to skip when copying a template. | ||||||
| var copyIgnoreDirectories = []string{".git", ".venv", "node_modules"} | ||||||
|
|
||||||
| // copyIgnoreFiles are files to skip when copying a template. | ||||||
| var copyIgnoreFiles = []string{".DS_Store"} | ||||||
|
|
||||||
| // CreateArgs are the arguments passed into the Create function | ||||||
| type CreateArgs struct { | ||||||
| AppName string | ||||||
| Template Template | ||||||
| GitBranch string | ||||||
| Subdir string | ||||||
| } | ||||||
|
|
||||||
| // Create will create a new Slack app on the file system and app manifest on the Slack API. | ||||||
|
|
@@ -121,8 +128,19 @@ func Create(ctx context.Context, clients *shared.ClientFactory, log *logger.Logg | |||||
| })) | ||||||
|
|
||||||
| // Create the project from a templateURL | ||||||
| if err := createApp(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, log, clients.Fs); err != nil { | ||||||
| return "", slackerror.Wrap(err, slackerror.ErrAppCreate) | ||||||
| subdir, err := normalizeSubdir(createArgs.Subdir) | ||||||
| if err != nil { | ||||||
| return "", err | ||||||
| } | ||||||
|
|
||||||
| if subdir != "" { | ||||||
| if err := createAppFromSubdir(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, subdir, log, clients.Fs); err != nil { | ||||||
| return "", slackerror.Wrap(err, slackerror.ErrAppCreate) | ||||||
| } | ||||||
| } else { | ||||||
| if err := createApp(ctx, projectDirPath, createArgs.Template, createArgs.GitBranch, log, clients.Fs); err != nil { | ||||||
| return "", slackerror.Wrap(err, slackerror.ErrAppCreate) | ||||||
| } | ||||||
|
Comment on lines
+136
to
+143
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: clean implementation choice!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪬 question(non-blocking): Would passing |
||||||
| } | ||||||
|
|
||||||
| // Change into the project directory to configure defaults and dependencies | ||||||
|
|
@@ -325,8 +343,8 @@ func createApp(ctx context.Context, dirPath string, template Template, gitBranch | |||||
| copyDirectoryOpts := goutils.CopyDirectoryOpts{ | ||||||
| Src: template.path, | ||||||
| Dst: dirPath, | ||||||
| IgnoreDirectories: []string{".git", ".venv", "node_modules"}, | ||||||
| IgnoreFiles: []string{".DS_Store"}, | ||||||
| IgnoreDirectories: copyIgnoreDirectories, | ||||||
| IgnoreFiles: copyIgnoreFiles, | ||||||
| } | ||||||
| if err := goutils.CopyDirectory(copyDirectoryOpts); err != nil { | ||||||
| return slackerror.Wrap(err, "error copying local template") | ||||||
|
|
@@ -343,6 +361,60 @@ func createApp(ctx context.Context, dirPath string, template Template, gitBranch | |||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // normalizeSubdir cleans the subdir path and returns "" if it resolves to root. | ||||||
| func normalizeSubdir(subdir string) (string, error) { | ||||||
|
Comment on lines
+364
to
+365
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: very nice! 👌🏻 |
||||||
| if subdir == "" { | ||||||
| return "", nil | ||||||
| } | ||||||
| cleaned := filepath.Clean(subdir) | ||||||
| if cleaned == "." || cleaned == "/" { | ||||||
| return "", nil | ||||||
| } | ||||||
|
Comment on lines
+369
to
+372
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: After This function appears to check if the file path is within the subtree and prevent traversal attacks where the path tries to access files outside of the root directory (template directory). I haven't used it personally, but it seems like a good security measure.
|
||||||
| if !filepath.IsLocal(cleaned) { | ||||||
| return "", slackerror.New(slackerror.ErrSubdirNotFound). | ||||||
| WithMessage("subdirectory path %q must be relative and within the template", subdir) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
🧮 quibble: Casing to errors as a complete sentence! |
||||||
| } | ||||||
| return cleaned, nil | ||||||
| } | ||||||
|
|
||||||
| // createAppFromSubdir clones the full template into a temp directory, then copies | ||||||
| // only the specified subdirectory to the final project path. | ||||||
| func createAppFromSubdir(ctx context.Context, dirPath string, template Template, gitBranch string, subdir string, log *logger.Logger, fs afero.Fs) error { | ||||||
| tmpDirRoot := afero.GetTempDir(fs, "") | ||||||
| tmpDir, err := afero.TempDir(fs, tmpDirRoot, "slack-create-") | ||||||
| if err != nil { | ||||||
| return slackerror.Wrap(err, "failed to create temporary directory") | ||||||
| } | ||||||
| defer func() { _ = fs.RemoveAll(tmpDir) }() | ||||||
|
|
||||||
| cloneDir := filepath.Join(tmpDir, "repo") | ||||||
| if err := createApp(ctx, cloneDir, template, gitBranch, log, fs); err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| subdirPath := filepath.Join(cloneDir, subdir) | ||||||
| info, err := fs.Stat(subdirPath) | ||||||
| if err != nil { | ||||||
| if os.IsNotExist(err) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I think this is okay, but if you want to mock the result during tests (to test both exist and not exist stats) then you can use the |
||||||
| return slackerror.New(slackerror.ErrSubdirNotFound). | ||||||
| WithMessage("subdirectory %q was not found in the template", subdir). | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
🚢 quibble: Casings to the sentence structure! |
||||||
| WithRemediation("Check that the path exists in the template at %q", template.GetTemplatePath()) | ||||||
|
Comment on lines
+399
to
+401
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL: |
||||||
| } | ||||||
| return slackerror.Wrap(err, "failed to access subdirectory") | ||||||
| } | ||||||
| if !info.IsDir() { | ||||||
| return slackerror.New(slackerror.ErrSubdirNotFound). | ||||||
| WithMessage("path %q in the template is not a directory", subdir) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
📠 quibble: Forbid this error be reached, but let's start with a capital path here. |
||||||
| } | ||||||
|
|
||||||
| return goutils.CopyDirectory(goutils.CopyDirectoryOpts{ | ||||||
| Src: subdirPath, | ||||||
| Dst: dirPath, | ||||||
| IgnoreDirectories: copyIgnoreDirectories, | ||||||
| IgnoreFiles: copyIgnoreFiles, | ||||||
| }) | ||||||
zimeg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
|
|
||||||
| // InstallProjectDependencies installs the project runtime dependencies or | ||||||
| // continues with next steps if that fails. You can specify the manifestSource | ||||||
| // for the project configuration file (default: ManifestSourceLocal) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,6 +225,7 @@ const ( | |
| ErrSocketConnection = "socket_connection_error" | ||
| ErrScopesExceedAppConfig = "scopes_exceed_app_config" | ||
| ErrStreamingActivityLogs = "streaming_activity_logs_error" | ||
| ErrSubdirNotFound = "subdir_not_found" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 thought(non-blocking): Would it make sense to group this alongside other template errors? To me the referenced Which is so close to |
||
| ErrSurveyConfigNotFound = "survey_config_not_found" | ||
| ErrSystemConfigIDNotFound = "system_config_id_not_found" | ||
| ErrSystemRequirementsFailed = "system_requirements_failed" | ||
|
|
@@ -1391,6 +1392,11 @@ Otherwise start your app for local development with: %s`, | |
| Message: "Failed to stream the most recent activity logs", | ||
| }, | ||
|
|
||
| ErrSubdirNotFound: { | ||
| Code: ErrSubdirNotFound, | ||
| Message: "The specified subdirectory was not found in the template repository", | ||
| }, | ||
|
|
||
| ErrSurveyConfigNotFound: { | ||
| Code: ErrSurveyConfigNotFound, | ||
| Message: "Survey config not found", | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏁 polish: To match surrounding error messages. Though I'm not confident in how consistent we are with this... Let's take note to include this in a Style Guide soon!