-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: cron parser #19
Changes from 14 commits
9442a9b
9fba9c0
93188f4
125e9a2
2b11319
e645c9d
83b96f2
2085e6e
8c5bdd2
04b5bac
2127c8b
b3e8e86
fa1eeb7
85b972e
c801e43
71d6ceb
d1aa972
6004c79
1aca175
63204c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package parser | ||
|
||
type parserConfig struct { | ||
cronFile string | ||
output string | ||
cronMatcher string | ||
hasUser bool | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package parser | ||
|
||
import ( | ||
"log" | ||
"regexp" | ||
"strings" | ||
) | ||
|
||
type cronLine struct { | ||
string | ||
} | ||
|
||
func (l cronLine) exportEnv() map[string]string { | ||
match := envRegex.FindStringSubmatch(l.string) | ||
answer := make(map[string]string) | ||
switch len(match) { | ||
case 0: | ||
case 3: | ||
answer[match[1]] = match[2] | ||
default: | ||
log.Panicf("found multiple(%d) env vars in single line\n please attach your crontab file too\n affected line: %s\n parser result: %#v\n", len(match), l.string, match) | ||
} | ||
return answer | ||
} | ||
|
||
func (l cronLine) exportSpec(regex *regexp.Regexp, env map[string]string, parser cronSpecParser) *cronSpec { | ||
match := regex.FindStringSubmatch(l.string) | ||
if len(match) < 1 { | ||
if len(strings.Trim(l.string, " \n\t")) == 0 { | ||
return nil | ||
} else { | ||
log.Panicf("cannot parse this non-empty line as cron specification: %s", l.string) | ||
} | ||
} | ||
return parser(match, env) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure robust parsing in The method - log.Panicf("cannot parse this non-empty line as cron specification: %s", l.string)
+ if len(strings.Trim(l.string, " \n\t")) != 0 {
+ log.Errorf("Failed to parse line as cron specification: %s", l.string)
+ return nil, fmt.Errorf("invalid cron specification: %s", l.string)
+ }
ToolsGitHub Check: codecov/patch
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package parser | ||
|
||
import ( | ||
"github.com/FMotalleb/crontab-go/config" | ||
) | ||
|
||
func (cfg parserConfig) parse() config.Config { | ||
cron := NewCronFromFile(cfg.cronFile) | ||
return *cron.ParseConfig( | ||
cfg.cronMatcher, | ||
cfg.hasUser, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add test coverage for the The method ToolsGitHub Check: codecov/patch
Ensure proper error handling in The method lacks explicit error handling. Consider modifying - cron := NewCronFromFile(cfg.cronFile)
- return *cron.ParseConfig(cfg.cronMatcher, cfg.hasUser)
+ cron, err := NewCronFromFile(cfg.cronFile)
+ if err != nil {
+ return nil, err
+ }
+ config, err := cron.ParseConfig(cfg.cronMatcher, cfg.hasUser)
+ if err != nil {
+ return nil, err
+ }
+ return config, nil
ToolsGitHub Check: codecov/patch
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package parser | ||
|
||
import ( | ||
"log" | ||
"regexp" | ||
) | ||
|
||
type ( | ||
cronSpecParser = func([]string, map[string]string) *cronSpec | ||
cronSpec struct { | ||
timing string | ||
user string | ||
command string | ||
environ map[string]string | ||
} | ||
) | ||
|
||
func normalParser(regex *regexp.Regexp) cronSpecParser { | ||
cronIndex := regex.SubexpIndex("cron") | ||
// userIndex := regex.SubexpIndex("user") | ||
cmdIndex := regex.SubexpIndex("cmd") | ||
if cronIndex < 0 || cmdIndex < 0 { | ||
log.Panicf("cannot find groups (cron,cmd) in regexp: `%s", regex) | ||
} | ||
return func(match []string, env map[string]string) *cronSpec { | ||
return &cronSpec{ | ||
timing: match[cronIndex], | ||
user: "", | ||
command: match[cmdIndex], | ||
environ: env, | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor error handling in The function - log.Panicf("cannot find groups (cron,cmd) in regexp: `%s", regex)
+ if cronIndex < 0 || cmdIndex < 0 {
+ return nil, fmt.Errorf("required groups not found in regex: %s", regex.String())
+ }
ToolsGitHub Check: codecov/patch
|
||
|
||
func withUserParser(regex *regexp.Regexp) cronSpecParser { | ||
cronIndex := regex.SubexpIndex("cron") | ||
userIndex := regex.SubexpIndex("user") | ||
cmdIndex := regex.SubexpIndex("cmd") | ||
if cronIndex < 0 || cmdIndex < 0 || userIndex < 0 { | ||
log.Panicf("cannot find groups (cron,user,cmd) in regexp: `%s", regex) | ||
} | ||
return func(match []string, env map[string]string) *cronSpec { | ||
return &cronSpec{ | ||
timing: match[cronIndex], | ||
user: match[userIndex], | ||
command: match[cmdIndex], | ||
environ: env, | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve error handling in Similar to - log.Panicf("cannot find groups (cron,user,cmd) in regexp: `%s", regex)
+ if cronIndex < 0 || cmdIndex < 0 || userIndex < 0 {
+ return nil, fmt.Errorf("required groups not found in regex: %s", regex.String())
+ }
ToolsGitHub Check: codecov/patch
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,170 @@ | ||
package parser | ||
|
||
import ( | ||
"fmt" | ||
"log" | ||
"os" | ||
"regexp" | ||
"strings" | ||
|
||
"github.com/FMotalleb/crontab-go/config" | ||
) | ||
|
||
var envRegex = regexp.MustCompile(`^(?<key>[\w\d_]+)=(?<value>.*)$`) | ||
|
||
type CronString struct { | ||
string | ||
} | ||
|
||
func NewCronString(cron string) CronString { | ||
return CronString{cron} | ||
} | ||
FMotalleb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
func NewCronFromFile(filePath string) CronString { | ||
file, err := os.OpenFile(filePath, os.O_RDONLY, 0o644) | ||
if err != nil { | ||
log.Panicf("can't open cron file: %v", err) | ||
} | ||
stat, err := file.Stat() | ||
if err != nil { | ||
log.Panicf("can't stat cron file: %v", err) | ||
} | ||
content := make([]byte, stat.Size()) | ||
_, err = file.Read(content) | ||
if err != nil { | ||
log.Panicf("can't open cron file: %v", err) | ||
} | ||
return CronString{string(content)} | ||
} | ||
|
||
func (s CronString) replaceAll(regex string, repl string) CronString { | ||
reg := regexp.MustCompile(regex) | ||
out := reg.ReplaceAllString(s.string, repl) | ||
return CronString{out} | ||
Comment on lines
+21
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for The function func TestReplaceAll(t *testing.T) {
cron := NewCronString("0 0 * * *")
result := cron.replaceAll(`\d`, "1")
expected := "1 1 * * *"
if result.string != expected {
t.Errorf("Expected %s, got %s", expected, result.string)
}
} ToolsGitHub Check: codecov/patch
|
||
} | ||
|
||
func (s CronString) sanitizeLineBreaker() CronString { | ||
return s.replaceAll( | ||
`\s*\\\s*\n\s*([\n|\n\s])*`, | ||
" ", | ||
) | ||
Comment on lines
+27
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for The function func TestSanitizeLineBreaker(t *testing.T) {
cron := NewCronString("0 0 * * * \\\n0 1 * * *")
result := cron.sanitizeLineBreaker()
expected := "0 0 * * * 0 1 * * *"
if result.string != expected {
t.Errorf("Expected %s, got %s", expected, result.string)
}
} ToolsGitHub Check: codecov/patch
|
||
} | ||
|
||
func (s CronString) sanitizeEmptyLine() CronString { | ||
return s.replaceAll( | ||
`\n\s*\n`, | ||
"\n", | ||
) | ||
Comment on lines
+34
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for The function func TestSanitizeEmptyLine(t *testing.T) {
cron := NewCronString("0 0 * * *\n\n0 1 * * *")
result := cron.sanitizeEmptyLine()
expected := "0 0 * * *\n0 1 * * *"
if result.string != expected {
t.Errorf("Expected %s, got %s", expected, result.string)
}
} ToolsGitHub Check: codecov/patch
|
||
} | ||
|
||
func (s CronString) sanitizeComments() CronString { | ||
return s.replaceAll( | ||
`\s*#.*`, | ||
"", | ||
) | ||
Comment on lines
+41
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for The function func TestSanitizeComments(t *testing.T) {
cron := NewCronString("0 0 * * * # comment")
result := cron.sanitizeComments()
expected := "0 0 * * *"
if result.string != expected {
t.Errorf("Expected %s, got %s", expected, result.string)
}
} ToolsGitHub Check: codecov/patch
|
||
} | ||
|
||
func (s CronString) sanitize() CronString { | ||
return s. | ||
replaceAll("\r\n", "\n"). | ||
sanitizeComments(). | ||
sanitizeLineBreaker(). | ||
sanitizeEmptyLine() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for The function func TestSanitize(t *testing.T) {
cron := NewCronString("0 0 * * * \\\n# comment\n\n0 1 * * *")
result := cron.sanitize()
expected := "0 0 * * * 0 1 * * *"
if result.string != expected {
t.Errorf("Expected %s, got %s", expected, result.string)
}
} ToolsGitHub Check: codecov/patch
|
||
} | ||
|
||
func (s CronString) lines() []string { | ||
return strings.Split(s.string, "\n") | ||
Comment on lines
+65
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for The function func TestLines(t *testing.T) {
cron := NewCronString("0 0 * * *\n0 1 * * *")
result := cron.lines()
expected := []string{"0 0 * * *", "0 1 * * *"}
for i, line := range result {
if line != expected[i] {
t.Errorf("Expected %s, got %s", expected[i], line)
}
}
} ToolsGitHub Check: codecov/patch
|
||
} | ||
|
||
func (s *CronString) parseAsSpec( | ||
pattern string, | ||
hasUser bool, | ||
) []cronSpec { | ||
envTable := make(map[string]string) | ||
specs := make([]cronSpec, 0) | ||
lines := s.sanitize().lines() | ||
matcher, parser := buildMapper(hasUser, pattern) | ||
|
||
for _, line := range lines { | ||
l := cronLine{line} | ||
if env := l.exportEnv(); len(env) > 0 { | ||
for key, val := range l.exportEnv() { | ||
if old, ok := envTable[key]; ok { | ||
log.Printf("env var of key `%s`, value `%s`, is going to be replaced by `%s`\n", key, old, val) | ||
} | ||
envTable[key] = val | ||
|
||
} | ||
} else { | ||
if spec := l.exportSpec(matcher, envTable, parser); spec != nil { | ||
specs = append(specs, *spec) | ||
} | ||
} | ||
} | ||
return specs | ||
} | ||
|
||
func (s *CronString) ParseConfig( | ||
pattern string, | ||
hasUser bool, | ||
) *config.Config { | ||
specs := s.parseAsSpec(pattern, hasUser) | ||
cfg := &config.Config{} | ||
for _, spec := range specs { | ||
addSpec(cfg, spec) | ||
} | ||
return cfg | ||
} | ||
|
||
func buildMapper(hasUser bool, pattern string) (*regexp.Regexp, func([]string, map[string]string) *cronSpec) { | ||
lineParser := "(?<cmd>.*)" | ||
if hasUser { | ||
lineParser = fmt.Sprintf(`(?<user>\w[\w\d]*)\s+%s`, lineParser) | ||
} | ||
cronLineMatcher := fmt.Sprintf(`^(?<cron>%s)\s+%s$`, pattern, lineParser) | ||
|
||
matcher, err := regexp.Compile(cronLineMatcher) | ||
if err != nil { | ||
log.Panicf("cannot parse cron `%s`", matcher) | ||
} | ||
var parser cronSpecParser | ||
if hasUser { | ||
parser = withUserParser(matcher) | ||
} else { | ||
parser = normalParser(matcher) | ||
} | ||
return matcher, parser | ||
} | ||
|
||
func addSpec(cfg *config.Config, spec cronSpec) { | ||
jobName := fmt.Sprintf("FromCron: %s", spec.timing) | ||
for _, job := range cfg.Jobs { | ||
if job.Name == jobName { | ||
task := config.Task{ | ||
Command: spec.command, | ||
UserName: spec.user, | ||
Env: spec.environ, | ||
} | ||
job.Tasks = append( | ||
job.Tasks, | ||
task, | ||
) | ||
return | ||
} | ||
} | ||
initJob(jobName, spec.timing, cfg) | ||
addSpec(cfg, spec) | ||
} | ||
|
||
func initJob(jobName string, timing string, cfg *config.Config) { | ||
job := &config.JobConfig{} | ||
job.Name = jobName | ||
job.Description = "Imported from cron file" | ||
job.Disabled = false | ||
job.Events = []config.JobEvent{ | ||
{ | ||
Cron: timing, | ||
}, | ||
} | ||
cfg.Jobs = append(cfg.Jobs, job) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification Lack of unit tests for critical functions. The functions
Analysis chainRobust functions for handling and parsing cron strings. The new functions introduced for handling cron strings are crucial for the parser's functionality. Consider replacing panics with more graceful error handling to make the module more robust and easier to integrate into other parts of the application. Additionally, ensure comprehensive testing, particularly for the regex operations and file handling aspects, to cover all edge cases and cron formats. Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that the cron string handling functions are covered by unit tests.
# Test: Search for tests covering the new functions in `cron_string.go`. Expect: Comprehensive coverage.
rg --type go 'NewCronString' -A 5 -B 5
rg --type go 'NewCronFromFile' -A 5 -B 5
rg --type go 'replaceAll' -A 5 -B 5
Length of output: 3454 Script: #!/bin/bash
# Description: Verify that the cron string handling functions are covered by unit tests.
# Test: Search for unit tests covering the new functions in `cron_string.go`. Expect: Comprehensive coverage.
rg --type go 'func Test' -l | xargs grep -E 'NewCronString|NewCronFromFile|replaceAll'
Length of output: 88 Toolsgolangci-lint
GitHub Check: codecov/patch
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
// Package parser manages holds the logic behind the sub command `parse` | ||
// this package is responsible for parsing a crontab file into valid config yaml file | ||
package parser | ||
|
||
import ( | ||
"bytes" | ||
"encoding/json" | ||
"errors" | ||
"io" | ||
"log" | ||
"os" | ||
|
||
"github.com/spf13/cobra" | ||
"gopkg.in/yaml.v3" | ||
) | ||
|
||
var ( | ||
cfg = &parserConfig{} | ||
ParserCmd = &cobra.Command{ | ||
Use: "parse <crontab file path>", | ||
ValidArgs: []string{"crontab file path"}, | ||
Short: "Parse crontab syntax and converts it into yaml syntax for crontab-go", | ||
Run: func(cmd *cobra.Command, args []string) { | ||
cfg.cronFile = cmd.Flags().Arg(0) | ||
log.SetPrefix("[Cron Parser]") | ||
if cfg.cronFile == "" { | ||
log.Panicln(errors.New("no crontab file specified, see usage using --help flag")) | ||
} | ||
finalConfig := cfg.parse() | ||
str, err := json.Marshal(finalConfig) | ||
if err != nil { | ||
log.Panicf("failed to marshal final config: %v", err) | ||
} | ||
hashMap := make(map[string]any) | ||
if err := json.Unmarshal(str, &hashMap); err != nil { | ||
log.Panicf("failed to unmarshal final config: %v", err) | ||
} | ||
ans, err := yaml.Marshal(hashMap) | ||
if err != nil { | ||
log.Panicf("failed to marshal final config: %v", err) | ||
} | ||
result := string(ans) | ||
log.Printf("output:\n%s", result) | ||
if cfg.output != "" { | ||
outputFile, err := os.OpenFile(cfg.output, os.O_WRONLY|os.O_CREATE, 0o644) | ||
if err != nil { | ||
log.Panicf("failed to open output file: %v", err) | ||
} | ||
buf := bytes.NewBufferString(result) | ||
_, err = io.Copy(outputFile, buf) | ||
if err != nil { | ||
log.Panicf("failed to write output file: %v", err) | ||
} | ||
} | ||
log.Println("Done writing output") | ||
os.Exit(0) | ||
}, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification Unit tests for The
Analysis chainComprehensive The Unit tests for The
Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that the `ParserCmd` and its functionalities are covered by unit tests.
# Test: Search for tests covering the `ParserCmd`. Expect: At least one test case for each major functionality.
rg --type go 'ParserCmd' -A 10 -B 10
Length of output: 2659 Script: #!/bin/bash
# Description: Verify if there are any test files covering `ParserCmd` directly or indirectly.
# Search for test files that might cover `ParserCmd`.
fd --type f --name '*_test.go' | xargs rg 'ParserCmd'
Length of output: 955 Script: #!/bin/bash
# Description: Verify if there are any test files covering `ParserCmd` directly or indirectly.
# Search for test files that might cover `ParserCmd`.
fd -e go -t f '_test' | xargs rg 'ParserCmd'
Length of output: 42 ToolsGitHub Check: codecov/patch
|
||
) | ||
|
||
func init() { | ||
ParserCmd.PersistentFlags().StringVarP(&cfg.output, "output", "o", "", "output file to write configuration to") | ||
ParserCmd.PersistentFlags().BoolVarP(&cfg.hasUser, "with-user", "u", false, "indicates that whether the given cron file has user field") | ||
ParserCmd.PersistentFlags().StringVar(&cfg.cronMatcher, "matcher", `(@(annually|yearly|monthly|weekly|daily|hourly|reboot))|(@every (\d+(ns|us|µs|ms|s|m|h))+)|((((\d+,)+\d+|(\d+(\/|-)\d+)|\d+|\*|(\*\/\d)) ?){5,7})`, "matcher for cron") | ||
} |
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.
Add test coverage and improve error handling for
exportEnv
.The method
exportEnv
uses regex to parse environment variables from a cron line. It's crucial to handle different cases robustly and to add tests to ensure its reliability, especially since it uses panic for error handling which is not recommended.Tools
GitHub Check: codecov/patch