Tool setup subcommand #1167
Tool setup subcommand #1167
Conversation
SummaryAdded Walkthrough
|
![bitrise[bot]](https://avatars.githubusercontent.com/in/32878?s=60&v=4){kind=small}
cli/tools_manager.go Outdated
| } | ||
| log.Printf("") | ||
| log.Infof("Run the following to apply environment changes:") | ||
| log.Printf(" export $(bitrise tools setup --from ... | grep export)") |
There was a problem hiding this comment.
🐛 Bug
The help text references a --from flag that doesn't exist. The actual flag is --config or -c as defined in the command flags at line 36.
🔄 Suggestion:
| log.Printf(" export $(bitrise tools setup --from ... | grep export)") | |
| log.Printf(" export $(bitrise tools setup --config ... | grep export)") |
| }, | ||
| cli.StringFlag{ | ||
| Name: "provider", | ||
| Usage: "Tool provider to use (asdf, mise). Default: mise", |
There was a problem hiding this comment.
IMHO we shouldn't expose it unless/until a valid usecase comes up. Let's just go with the current default provider used elsewhere.
| Usage: "Workflow ID to use when installing from bitrise config (optional, uses global tools if not specified)", | ||
| }, | ||
| cli.BoolFlag{ | ||
| Name: "fast-install", |
There was a problem hiding this comment.
Same as above, I'd rather stick to the default for now, then iterate on it if needed
| | ||
| var toolsCommand = cli.Command{ | ||
| Name: "tools", | ||
| Usage: "Manage development tools.", |
There was a problem hiding this comment.
| Usage: "Manage development tools.", | |
| Usage: "Manage available tools from inside the workflow.", |
| var pluginURL *string | ||
| if opts.ExtraPlugins != nil { | ||
| if url, ok := opts.ExtraPlugins[models.ToolID(tool.ToolName)]; ok { | ||
| pluginURL = &url |
There was a problem hiding this comment.
🐛 Bug
Taking the address of a loop variable that goes out of scope. The pointer will be invalid after the if block completes, leading to undefined behavior when the pluginURL is later dereferenced.
🔄 Suggestion:
| pluginURL = &url | |
| urlCopy := url | |
| pluginURL = &urlCopy |
| if bitriseConfigPath != "" { | ||
| config, warnings, err := CreateBitriseConfigFromCLIParams("", bitriseConfigPath, bitrise.ValidationTypeFull) | ||
| if err != nil { | ||
| return fmt.Errorf("load config: %w", err) | ||
| } | ||
| | ||
| for _, warning := range warnings { | ||
| log.Warnf("Config warning: %s", warning) | ||
| } | ||
| | ||
| tracker := analytics.NewDefaultTracker() | ||
| envs, err := toolprovider.Run(config, tracker, false, workflowID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| | ||
| output, err := convertToOutputFormat(envs, format) | ||
| if err != nil { | ||
| return fmt.Errorf("convert to output format: %w", err) | ||
| } | ||
| fmt.Println(output) | ||
| } | ||
| | ||
| opts := toolprovider.SetupOptions{ | ||
| VersionFiles: configFiles, | ||
| ProviderName: provider, | ||
| ExperimentalFastInstall: fastInstall, | ||
| } | ||
| | ||
| tracker := analytics.NewDefaultTracker() | ||
| envs, err := toolprovider.SetupFromVersionFiles(opts, tracker) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| | ||
| output, err := convertToOutputFormat(envs, format) | ||
| if err != nil { | ||
| return fmt.Errorf("convert to output format: %w", err) | ||
| } | ||
| // TODO: avoid printing other log output when --bash or --json is used | ||
| fmt.Println(output) | ||
| return nil |
There was a problem hiding this comment.
🐛 Bug
The function processes a bitrise config if found (lines 118-139), but then unconditionally calls toolprovider.SetupFromVersionFiles with the same configFiles list (lines 141-159). This means both code paths execute when a bitrise config is found, causing duplicate processing and output. Additionally, the bitrise config file will be passed to SetupFromVersionFiles which expects version files, potentially causing errors. The logic should likely use an if-else structure or return after processing the bitrise config.
🤖 Prompt for AI Agents:
In cli/tools_manager.go between lines 118 and 159, Fix the logic to ensure only one code path executes: either process the bitrise config OR process version files, not both. Consider using an if-else structure or returning after processing the bitrise config. 
Checklist
README.mdis updated with the changes (if needed)Version
Requires a MAJOR/MINOR/PATCH version update
Context
Changes
Investigation details
Decisions