Skip to content
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.

Sign up for GitHub

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

Bugfixes: low hanging fruit #55

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Bugfixes: low hanging fruit #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bcd6144
brighten white logs
bohendo Jul 26, 2024
8bc0dd4
add '/' to the end of the bucket deletion prefix
bohendo Jul 26, 2024
1595eb8
use flag to get launch config path instead of positional param
bohendo Jul 26, 2024
979c567
rm old positional arg logic from config flag check
bohendo Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/cloudexec/cancel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
func CancelJob(config config.Config, existingState *state.State, job *state.Job, force bool) error {
if job.Status != state.Provisioning && job.Status != state.Running {
log.Info("Job %v is not running, it is %s", job.ID, job.Status)
return nil
return nil
}
log.Warn("Destroying droplet %s associated with job %v: IP=%v | CreatedAt=%s", job.Droplet.Name, job.ID, job.Droplet.IP, job.Droplet.Created)
if !force { // Ask for confirmation before cleaning this job if no force flag
Expand Down
2 changes: 1 addition & 1 deletion cmd/cloudexec/clean.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
)

func CleanJob(config config.Config, existingState *state.State, jobID int64, force bool) error {
prefix := fmt.Sprintf("job-%v", jobID)
prefix := fmt.Sprintf("job-%v/", jobID)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add a / to the end of the deletion prefix so that deleting job-1 doesn't also delete from job-10, etc

Copy link

Choose a reason for hiding this comment

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

Image description CodeRabbit

The change from job-%v to job-%v/ in the prefix string format is significant. This change will affect how objects are listed from S3. If the intention is to list all objects under a specific job directory, this change is correct. However, if the intention was to list objects that start with job-{jobID}, this change could cause unexpected behavior.

- prefix := fmt.Sprintf("job-%v", jobID)
+ prefix := fmt.Sprintf("job-%v/", jobID)

Please ensure that this change aligns with the intended functionality.

objects, err := s3.ListObjects(config, prefix)
if err != nil {
return fmt.Errorf("Failed to list objects in bucket with prefix %s: %w", prefix, err)
Expand Down
23 changes: 10 additions & 13 deletions cmd/cloudexec/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@ import (
)

var (
Version = "dev"
Commit = "none"
Date = "unknown"
ConfigFilePath = fmt.Sprintf("%s/.config/cloudexec/config.toml", os.Getenv("HOME"))
LaunchConfigFilePath = "./cloudexec.toml"
Version = "dev"
Commit = "none"
Date = "unknown"
ConfigFilePath = fmt.Sprintf("%s/.config/cloudexec/config.toml", os.Getenv("HOME"))
Comment on lines +17 to +20
Copy link

Choose a reason for hiding this comment

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

Image description CodeRabbit

The removal of LaunchConfigFilePath from the global variables and its addition as a default value for the config flag in the CLI command is a good change. It makes the code more modular and less dependent on global state. However, it's important to note that using os.Getenv("HOME") directly can lead to issues if the environment variable is not set. Consider using os.UserHomeDir() instead which handles more edge cases.

- ConfigFilePath = fmt.Sprintf("%s/.config/cloudexec/config.toml", os.Getenv("HOME"))
+ homeDir, _ := os.UserHomeDir()
+ ConfigFilePath = fmt.Sprintf("%s/.config/cloudexec/config.toml", homeDir)

)

func main() {
Expand Down Expand Up @@ -90,6 +89,7 @@ func main() {
Flags: []cli.Flag{
&cli.StringFlag{
Name: "config",
Value: "./cloudexec.toml", // default config filepath
Usage: "cloudexec.toml file path",
},
&cli.StringFlag{
Expand All @@ -109,15 +109,12 @@ func main() {
return configErr
}
// Check if a local cloudexec.toml exists
if _, err := os.Stat(LaunchConfigFilePath); os.IsNotExist(err) {
// Check if the path to a launch config is provided
if c.Args().Len() < 1 {
return fmt.Errorf("please provide a path to a cloudexec.toml file or create one in the current directory")
}
LaunchConfigFilePath = c.Args().Get(0)
launchConfigFilePath := c.String("config")
if _, err := os.Stat(launchConfigFilePath); os.IsNotExist(err) {
return fmt.Errorf("please create cloudexec.toml with 'cloudexec init' or use the '--config' flag to provide a path to your custom launch config file")
}
// Load the launch configuration
lc, err := LoadLaunchConfig(LaunchConfigFilePath)
lc, err := LoadLaunchConfig(launchConfigFilePath)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The config flag was defined, mentioned in help text, but was completely ignored. It might surprise you by trying to load a launch config from the first positional arg, but that wouldn't work if other flags are provided first.

This change switches to using the --config flag as you'd expect based on help text.

Comment on lines +112 to +117
Copy link

Choose a reason for hiding this comment

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

Image description CodeRabbit

The error handling here could be improved. Currently, if os.Stat fails for a reason other than the file not existing (e.g., insufficient permissions), the error message will still suggest that the file does not exist. It would be better to handle these cases separately.

- if _, err := os.Stat(launchConfigFilePath); os.IsNotExist(err) {
+ if _, err := os.Stat(launchConfigFilePath); err != nil {
+   if os.IsNotExist(err) {
      return fmt.Errorf("please create cloudexec.toml with 'cloudexec init' or use the '--config' flag to provide a path to your custom launch config file")
+   } else {
+     return fmt.Errorf("error checking for cloudexec.toml: %v", err)
+   }
+ }

if err != nil {
return err
}
Expand All @@ -129,7 +126,7 @@ func main() {
return err
}
err = Launch(config, dropletSize, dropletRegion, lc)
return err
return err
},
},

Expand Down
2 changes: 1 addition & 1 deletion justfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fmt:
go fmt pkg/ssh/*.go
go fmt pkg/state/*.go

trunk:
trunk: fmt
trunk fmt
trunk check
Comment on lines +8 to 10
Copy link

Choose a reason for hiding this comment

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

Image description CodeRabbit

The change made to the justfile seems to be renaming the trunk command to trunk: fmt. This could potentially break any scripts or processes that rely on the trunk command. If the intention was to add a new command fmt under trunk, it should be done like this:

- trunk: fmt
+ trunk-fmt:

This way, the original trunk command remains intact and a new command trunk-fmt is added.


Expand Down
2 changes: 1 addition & 1 deletion pkg/log/log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const (
ColorGreen = "\033[32m"
ColorBlue = "\033[34m"
ColorYellow = "\033[33m"
ColorWhite = "\033[37m"
ColorWhite = "\033[97m"
Copy link

Choose a reason for hiding this comment

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

Image description CodeRabbit

The change from "\033[37m" to "\033[97m" for ColorWhite alters the color code for white. While "\033[37m" represents a standard white, "\033[97m" is a bright white. This might make the white text too bright or unreadable on some terminals with light backgrounds. It would be better to ensure compatibility across different terminal themes.

- 	ColorWhite  = "\033[97m"
+ 	ColorWhite  = "\033[37m"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the color control code for "bright white" which looks an awful lot like default white tbh, either way: it's brighter and easier to read than the gray.

)

func Info(msg string, args ...interface{}) {
Expand Down
Loading