-
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
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 |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ var createTemplateURLFlag string | |
| var createGitBranchFlag string | ||
| var createAppNameFlag string | ||
| var createListFlag bool | ||
| var createSubdirFlag string | ||
|
|
||
| // Handle to client's create function used for testing | ||
| // TODO - Find best practice, such as using an Interface and Struct to create a client | ||
|
|
@@ -66,6 +67,7 @@ name your app 'agent' (not create an AI Agent), use the --name flag instead.`, | |
| {Command: "create agent my-agent-app", Meaning: "Create a new AI Agent app"}, | ||
| {Command: "create my-project -t slack-samples/deno-hello-world", Meaning: "Start a new project from a specific template"}, | ||
| {Command: "create --name my-project", Meaning: "Create a project named 'my-project'"}, | ||
| {Command: "create my-project -t org/monorepo --subdir apps/my-app", Meaning: "Create from a subdirectory of a template"}, | ||
| }), | ||
| Args: cobra.MaximumNArgs(2), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
|
|
@@ -79,6 +81,7 @@ name your app 'agent' (not create an AI Agent), use the --name flag instead.`, | |
| cmd.Flags().StringVarP(&createGitBranchFlag, "branch", "b", "", "name of git branch to checkout") | ||
| cmd.Flags().StringVarP(&createAppNameFlag, "name", "n", "", "name for your app (overrides the name argument)") | ||
| cmd.Flags().BoolVar(&createListFlag, "list", false, "list available app templates") | ||
| cmd.Flags().StringVar(&createSubdirFlag, "subdir", "", "subdirectory within the template to use as project root") | ||
|
|
||
| return cmd | ||
| } | ||
|
|
@@ -141,6 +144,7 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args [] | |
| AppName: appNameArg, | ||
| Template: template, | ||
| GitBranch: createGitBranchFlag, | ||
| Subdir: createSubdirFlag, | ||
|
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: Right now Personally, I'd learn toward erroring if |
||
| } | ||
| clients.EventTracker.SetAppTemplate(template.GetTemplatePath()) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,7 @@ 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 +122,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
+130
to
+137
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! |
||
| } | ||
|
|
||
| // Change into the project directory to configure defaults and dependencies | ||
|
|
@@ -343,6 +355,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
+358
to
+359
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
+363
to
+366
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 strings.HasPrefix(cleaned, "..") || filepath.IsAbs(cleaned) { | ||
| return "", slackerror.New(slackerror.ErrSubdirNotFound). | ||
| WithMessage("subdirectory path %q must be relative and within the template", subdir) | ||
| } | ||
| 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 { | ||
| tmpDir, err := os.MkdirTemp("", "slack-create-*") | ||
|
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: We should not use Instead, we should use our I imagine we can use tempDirRoot := afero.GetTempDir(fs, "")
tempDirPath, err := afero.TempDir(fs, tmpDirRoot, "slack-create-") |
||
| if err != nil { | ||
| return slackerror.Wrap(err, "failed to create temporary directory") | ||
| } | ||
| // Remove so createApp can create it fresh (go-git requires non-existent target) | ||
| os.Remove(tmpDir) | ||
|
Comment on lines
+381
to
+382
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: Why do we make the temp directory on L377 and then immediately delete it? |
||
| defer os.RemoveAll(tmpDir) | ||
|
Comment on lines
+382
to
+383
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: Likewise, here I think we should use |
||
|
|
||
| if err := createApp(ctx, tmpDir, template, gitBranch, log, fs); err != nil { | ||
| return err | ||
| } | ||
|
Comment on lines
+385
to
+387
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: Clever and clean approach! |
||
|
|
||
| subdirPath := filepath.Join(tmpDir, subdir) | ||
| info, err := os.Stat(subdirPath) | ||
|
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: If we're using the |
||
| 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). | ||
| WithRemediation("Check that the path exists in the template at %q", template.GetTemplatePath()) | ||
|
Comment on lines
+393
to
+395
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) | ||
| } | ||
|
|
||
| return goutils.CopyDirectory(goutils.CopyDirectoryOpts{ | ||
| Src: subdirPath, | ||
| Dst: dirPath, | ||
| IgnoreDirectories: []string{".git", ".venv", "node_modules"}, | ||
| IgnoreFiles: []string{".DS_Store"}, | ||
| }) | ||
|
Comment on lines
+404
to
+409
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 noticed that we have the same |
||
| } | ||
|
|
||
| // 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) | ||
|
|
||

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.
suggestion: An attempt to shorten the description so that it doesn't wrap on 80 character terminals
Before:

After:
