feat: add furyctl get aliases command for tool management#618
feat: add furyctl get aliases command for tool management#618nutellinoit wants to merge 11 commits intomainfrom
furyctl get aliases command for tool management#618Conversation
355b238 to
e3afc42
Compare
ralgozino
left a comment
There was a problem hiding this comment.
I left some comments, the mise integration is not working for me but could be something on my end, I'll investigate and report back.
- Add new tools command with aliases, functions, and mise subcommands - Implement mise integration for tool version management - Add comprehensive documentation and test coverage - Update linting configuration and Makefile - Add README section for tool aliases usage - Include internal improvements for file getter functionality
5e3bf12 to
0c86d80
Compare
…patibility Update comment to emphasize that aliases point to tools with versions compatible with SIGHUP Distribution configuration rather than just scanning the .furyctl/bin directory. Co-authored-by: Ramiro Algozino <ramiro@sighup.io> Signed-off-by: Samuele Chiocca <samuele@sighup.io>
Replace custom FilePermissions constant with iox.FullRWPermAccess which has the same value (0o600) but is already defined in the internal/x/io package. Co-authored-by: Ramiro Algozino <ramiro@sighup.io> Signed-off-by: Samuele Chiocca <samuele@sighup.io>
Remove the dot prefix from .mise.toml to match the current standard mise.toml filename format. Co-authored-by: Ramiro Algozino <ramiro@sighup.io> Signed-off-by: Samuele Chiocca <samuele@sighup.io>
Add --mise-file flag to allow custom mise configuration file paths. This supports: - Different directories for mise.toml - Legacy .mise.toml format - Global mise config (~/.config/mise/config.toml) Replace hard-coded "mise.toml" constant with configurable parameter passed to updateMiseConfig and RevertMiseConfig functions. Co-authored-by: Ramiro Algozino <ramiro@sighup.io> Signed-off-by: Samuele Chiocca <samuele@sighup.io>
Add proper section order preservation to maintain the original structure and ordering of mise.toml files: - Add SectionOrder field to MiseConfig struct - Extract section order during file parsing using regex - Preserve original section order when saving the file - Add automatic 'mise fmt' execution after saving - Update comments to reflect actual behavior This prevents variables that depend on previous sections from breaking and maintains user's preferred file organization. Both adding and removing tools preserve the original section order. Co-authored-by: Ramiro Algozino <ramiro@sighup.io> Signed-off-by: Samuele Chiocca <samuele@sighup.io>
Remove all logrus.SetLevel() calls that suppress user's debug logs during distribution download, configuration validation, and dependency downloads. Users who set debug logging should see all debug information, not have it suppressed for "clean output". This gives users full control over their logging preferences. Co-authored-by: Ramiro Algozino <ramiro@sighup.io> Signed-off-by: Samuele Chiocca <samuele@sighup.io>
Revert the get command short description back to the original wording as the change was flagged as potentially unwanted in the PR review. Co-authored-by: Ramiro Algozino <ramiro@sighup.io> Signed-off-by: Samuele Chiocca <samuele@sighup.io>
Update all RevertMiseConfig test calls to include the new miseFile parameter that was added to support configurable mise file paths. Co-authored-by: Ramiro Algozino <ramiro@sighup.io>
Fix whitespace and formatting issues in order preservation code: - Add blank line after variable declarations (wsl linter) - Fix comment placement in error handling blocks (wsl linter) - Add blank line before return statement in multi-line blocks (wsl linter) - Remove trailing whitespace in control structures (gci linter) Co-Authored-By: ralgozino <ralgozino@users.noreply.github.com>
Remove extra blank lines after variable assignments to resolve WSL (White Space Linter) violations. These fixes were automatically applied during the make format-go process. Co-Authored-By: Ramiro Algozino <ramiro@sighup.io>
| func TestAliasesCmd_FlagValidation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cmd := tools.NewAliasesCmd() | ||
|
|
||
| // Test that command can be created and has expected flags. | ||
| assert.NotNil(t, cmd) | ||
|
|
||
| // Verify required flags exist. | ||
| configFlag := cmd.Flags().Lookup("config") | ||
| assert.NotNil(t, configFlag) | ||
| assert.Equal(t, "furyctl.yaml", configFlag.DefValue) | ||
|
|
||
| binPathFlag := cmd.Flags().Lookup("bin-path") | ||
| assert.NotNil(t, binPathFlag) | ||
|
|
||
| skipDepsFlag := cmd.Flags().Lookup("skip-deps-download") | ||
| assert.NotNil(t, skipDepsFlag) | ||
| assert.Equal(t, "false", skipDepsFlag.DefValue) | ||
| } |
There was a problem hiding this comment.
not sure how much this kind of test makes sense. it's testing more cobra than our actual code, a better test would be to actually use test the flags in the actual code
| func TestFunctionsCmd_FlagValidation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cmd := tools.NewFunctionsCmd() | ||
|
|
||
| // Test that command can be created and has expected flags. | ||
| assert.NotNil(t, cmd) | ||
|
|
||
| // Verify required flags exist. | ||
| configFlag := cmd.Flags().Lookup("config") | ||
| assert.NotNil(t, configFlag) | ||
| assert.Equal(t, "furyctl.yaml", configFlag.DefValue) | ||
|
|
||
| binPathFlag := cmd.Flags().Lookup("bin-path") | ||
| assert.NotNil(t, binPathFlag) | ||
|
|
||
| skipDepsFlag := cmd.Flags().Lookup("skip-deps-download") | ||
| assert.NotNil(t, skipDepsFlag) | ||
| assert.Equal(t, "false", skipDepsFlag.DefValue) | ||
| } |
| func TestMiseCmd_RevertFlagValidation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cmd := tools.NewMiseCmd() | ||
|
|
||
| // Verify revert flag exists. | ||
| revertFlag := cmd.Flags().Lookup("revert") | ||
| assert.NotNil(t, revertFlag) | ||
| assert.Equal(t, "false", revertFlag.DefValue) | ||
| assert.Equal(t, "Remove furyctl-managed tools from mise.toml instead of adding them", revertFlag.Usage) | ||
|
|
||
| // Verify force flag exists. | ||
| forceFlag := cmd.Flags().Lookup("force") | ||
| assert.NotNil(t, forceFlag) | ||
| assert.Equal(t, "false", forceFlag.DefValue) | ||
| assert.Equal(t, "Skip confirmation prompt when reverting tools", forceFlag.Usage) | ||
| } |
| } | ||
| } | ||
|
|
||
| func TestRevertMiseConfig_NoFile(t *testing.T) { //nolint:paralleltest // Test changes working directory |
There was a problem hiding this comment.
why are these tests not using t.parallel() and are annotated with nolint:paralleltest instead?
| if needsDownload { | ||
| if flags.SkipDepsDownload { | ||
| return nil, ErrNoToolsFound | ||
| } | ||
|
|
||
| // Download distribution and dependencies. | ||
| var distrodl *dist.Downloader | ||
| if flags.DistroLocation == "" { | ||
| distrodl = dist.NewCachingDownloader(client, outDir, typedGitProtocol, "") | ||
| } else { | ||
| distrodl = dist.NewDownloader(client, typedGitProtocol, "") | ||
| } | ||
|
|
||
| // Validate base requirements. | ||
| depsvl := dependencies.NewValidator(executor, binPath, furyctlPath, false) | ||
| if err := depsvl.ValidateBaseReqs(); err != nil { | ||
| return nil, fmt.Errorf("error while validating requirements: %w", err) | ||
| } | ||
|
|
||
| // Download the distribution. | ||
| res, err := distrodl.Download(flags.DistroLocation, furyctlPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error while downloading distribution: %w", err) | ||
| } | ||
|
|
||
| basePath := path.Join(outDir, ".furyctl", res.MinimalConf.Metadata.Name) | ||
|
|
||
| // Validate the furyctl.yaml file. | ||
| if err := config.Validate(furyctlPath, res.RepoPath); err != nil { | ||
| return nil, fmt.Errorf("error while validating configuration file: %w", err) | ||
| } | ||
|
|
||
| // Download the dependencies. | ||
| depsdl := dependencies.NewCachingDownloader(client, outDir, basePath, binPath, typedGitProtocol) | ||
|
|
||
| if _, err := depsdl.DownloadTools(res.DistroManifest); err != nil { | ||
| return nil, fmt.Errorf("error while downloading tools: %w", err) | ||
| } | ||
|
|
||
| distroManifest = res.DistroManifest | ||
| } else { | ||
| // Load the distribution to get tool versions from kfd.yaml. | ||
| var distrodl *dist.Downloader | ||
| if flags.DistroLocation == "" { | ||
| distrodl = dist.NewCachingDownloader(client, outDir, typedGitProtocol, "") | ||
| } else { | ||
| distrodl = dist.NewDownloader(client, typedGitProtocol, "") | ||
| } | ||
|
|
||
| res, err := distrodl.Download(flags.DistroLocation, furyctlPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error while downloading distribution: %w", err) | ||
| } | ||
|
|
||
| distroManifest = res.DistroManifest | ||
| } |
There was a problem hiding this comment.
this big if/else block could probably be refactored in a better way with proper functions that say what they are doing
| case "aliases": | ||
| assert.Equal(t, "Generate bash aliases for downloaded tools", subcmd.Short) | ||
| assert.Contains(t, subcmd.Long, "bash aliases") | ||
|
|
||
| case "functions": | ||
| assert.Equal(t, "Generate bash functions for downloaded tools", subcmd.Short) | ||
| assert.Contains(t, subcmd.Long, "bash functions") | ||
| assert.Contains(t, subcmd.Long, "Functions have higher") | ||
|
|
||
| case "mise": | ||
| assert.Equal(t, "Generate or update mise.toml with downloaded tool paths", subcmd.Short) | ||
| assert.Contains(t, subcmd.Long, "mise.toml") | ||
| } |
There was a problem hiding this comment.
testing for descriptions seems overly verbose, do we really need it?
Summary 💡
Adds
furyctl toolscommand supporting multiple shell integration formats (aliases, functions, mise).Closes: #591
Description 📝
Command Structure:
furyctl tools aliases- Generate bash aliasesfuryctl tools functions- Generate bash functions (higher precedence than aliases)furyctl tools mise- Generate/update mise.toml configurationCore Features:
--outdirflagImplementation:
Usage Examples:
Key Technical Changes:
cmd/tools/shared.gogithub.com/pelletier/go-toml/v2Breaking Changes 💔
None.
Tests performed 🧪