Skip to content

Add shell package into general utils#33

Merged
magerstam merged 1 commit intomainfrom
dev_feiyu3
Jun 5, 2025
Merged

Add shell package into general utils#33
magerstam merged 1 commit intomainfrom
dev_feiyu3

Conversation

@feiyu11859661
Copy link
Copy Markdown
Contributor

Merge Checklist

All boxes should be checked before merging the PR

  • [] The changes in the PR have been built and tested
  • [] Ready to merge

Description

Any Newly Introduced Dependencies

How Has This Been Tested?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new internal shell utility package to resolve command binaries by full path, manage environment variables, and provide flexible command execution (with sudo, chroot, streaming, and input support).

  • Defines a commandMap for absolute binary lookups and implements verifyCmdWithFullPath
  • Provides helpers for fetching OS environments and proxy settings
  • Adds GetFullCmdStr, ExecCmd, ExecCmdWithStream, and ExecCmdWithInput functions
Comments suppressed due to low confidence (2)

internal/utils/general/shell/shell.go:122

  • Function name IsCommandExist suggests a boolean return, but it returns a string path. Consider renaming to GetCommandPath or changing the signature to return a bool.
func IsCommandExist(cmd string, chrootPath string) string {

internal/utils/general/shell/shell.go:1

  • [nitpick] The new shell package lacks unit tests. Adding tests for key functions like GetFullCmdStr, ExecCmdWithStream, and verifyCmdWithFullPath would improve reliability.
package shell

Comment thread internal/utils/general/shell/shell.go
Comment thread internal/utils/general/shell/shell.go Outdated
Comment thread internal/utils/general/shell/shell.go
Copy link
Copy Markdown
Contributor

@magerstam magerstam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks nice and clean, just a suggestion on the hard coded map for commands.

@magerstam magerstam merged commit e0b359f into main Jun 5, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants