Bugfixes: low hanging fruit #55
Conversation
|
WalkthroughThis pull request introduces changes to the handling of configuration files in Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
| @@ -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) | |||
There was a problem hiding this comment.
Add a / to the end of the deletion prefix so that deleting job-1 doesn't also delete from job-10, etc
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (5)
- cmd/cloudexec/cancel.go (1)
- cmd/cloudexec/clean.go (1)
- cmd/cloudexec/main.go (4)
- justfile (1)
- pkg/log/log.go (1)
Files skipped from review due to trivial changes (1)
- cmd/cloudexec/cancel.go
Review comments generated (7)
- Review: 5
- LGTM: 2
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -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) | |||
There was a problem hiding this comment.
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.
| @@ -10,7 +10,7 @@ const ( | |||
| ColorGreen = "\033[32m" | |||
| ColorBlue = "\033[34m" | |||
| ColorYellow = "\033[33m" | |||
| ColorWhite = "\033[37m" | |||
| ColorWhite = "\033[97m" | |||
There was a problem hiding this comment.
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"| trunk: fmt | ||
| trunk fmt | ||
| trunk check |
There was a problem hiding this comment.
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.
| Version = "dev" | ||
| Commit = "none" | ||
| Date = "unknown" | ||
| ConfigFilePath = fmt.Sprintf("%s/.config/cloudexec/config.toml", os.Getenv("HOME")) |
There was a problem hiding this comment.
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)
cmd/cloudexec/main.go
Outdated
| launchConfigFilePath := c.String("config") | ||
| 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") | ||
| 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") | ||
| } | ||
| LaunchConfigFilePath = c.Args().Get(0) | ||
| launchConfigFilePath = c.String("config") | ||
| } | ||
| // Load the launch configuration | ||
| lc, err := LoadLaunchConfig(LaunchConfigFilePath) | ||
| lc, err := LoadLaunchConfig(launchConfigFilePath) |
There was a problem hiding this comment.
There seems to be a redundant line at 118 where launchConfigFilePath is assigned the same value again from c.String("config"). This is unnecessary as it doesn't change between lines 112 and 118. You should remove this redundancy.
- launchConfigFilePath = c.String("config")
| @@ -10,7 +10,7 @@ const ( | |||
| ColorGreen = "\033[32m" | |||
| ColorBlue = "\033[34m" | |||
| ColorYellow = "\033[33m" | |||
| ColorWhite = "\033[37m" | |||
| ColorWhite = "\033[97m" | |||
There was a problem hiding this comment.
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.
| } | ||
| // Load the launch configuration | ||
| lc, err := LoadLaunchConfig(LaunchConfigFilePath) | ||
| lc, err := LoadLaunchConfig(launchConfigFilePath) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Files selected (1)
- cmd/cloudexec/main.go (4)
Review comments generated (4)
- Review: 1
- LGTM: 3
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| 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) |
There was a problem hiding this comment.
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)
+ }
+ }
This PR targets 3 small, unrelated bugs that were reported recently.
resolves #50 #52 #54
Summary by CodeRabbit
cancel.goandmain.goby adjusting indentation and handling of configuration file paths.fmtoption to thetrunkcommand in thejustfile, enhancing user control over formatting.log.gofor better visibility in console output.