-
Notifications
You must be signed in to change notification settings - Fork 2
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
TOOLS-72 TOOLS-75 external tools validation #15
Conversation
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.
Looks great (with some minor notes)
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.
Looks good, minor changes requested.
internal/commands/base.go
Outdated
//Check Docker and SSM Agent | ||
_, err = CheckCommand("docker", []string{"info"}) | ||
if err != nil { | ||
return errors.New("docker is not running or is not installed") |
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.
Let's
- Capitalize Docker
- Provide a link to Docker download/install instructions.
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.
Also let's provide to ssm plug-in installation a link in the place where we notify user to install the plugin manually.
internal/commands/base.go
Outdated
|
||
err = DownloadSSMAgentPlugin() | ||
if err != nil { | ||
return fmt.Errorf("download SSM Agent error: %v", err) |
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.
Let's capitalize Download
internal/commands/base.go
Outdated
|
||
err = CleanupSSMAgent() | ||
if err != nil { | ||
return fmt.Errorf("cleanup SSM Agent error: %v", err) |
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.
Let's capitalize first letter
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.
@@ -14,7 +14,7 @@ func (c *izeBuilderCommon) initConfig(filename string) (*config.Config, error) { | |||
} | |||
|
|||
if path == "" { | |||
return nil, errors.New("A Waypoint configuration file (waypoint.hcl) is required but wasn't found.") | |||
return nil, errors.New("an Ize configuration file (ize.hcl) is required but wasn't 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.
Let's capitalize first letter
@@ -23,7 +23,7 @@ func (c *izeBuilderCommon) initConfig(filename string) (*config.Config, error) { | |||
func (c *izeBuilderCommon) initConfigPath(filename string) (string, error) { | |||
path, err := config.FindPath(filename) | |||
if err != nil { | |||
return "", fmt.Errorf("Error looking for a Ize configuration: %s", err) | |||
return "", fmt.Errorf("error looking for a Ize configuration: %s", err) |
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.
Is there a reason why we are switching to lowercase in the first word?
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.
internal/commands/helpers.go
Outdated
if runtime.GOOS == "darwin" { | ||
command = []string{"rm", "-rf", "sessionmanager-bundle sessionmanager-bundle.zip"} | ||
} else if runtime.GOOS == "linux" { | ||
command = []string{"rm", "-rf", "session-manager-plugin.rpm"} |
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.
Not sure if we need -r here, it's just a file
ba54234
to
3da92c8
Compare
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.
LGTM
This commits adds:
Example:
Windows 11 SSM Agent
Ubuntu (wsl2) SSM Agent
Ubuntu (wsl2) Docker
Windows 11 Docker
Not tested on MasOS