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.

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

Merged
merged 20 commits into from
Jun 30, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
15 changes: 14 additions & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,19 @@
"-c",
"config.local.yaml"
]
}
},
{
"name": "Parse",
"type": "go",
"request": "launch",
"mode": "auto",
"program": ".",
"env": {},
"args": [
"parse",
"crontab.log",
"-u"
]
},
]
}
8 changes: 8 additions & 0 deletions cmd/parser/config.go
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
}
41 changes: 41 additions & 0 deletions cmd/parser/cron_line.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package parser

import (
"fmt"
"regexp"
"strings"
)

type cronLine struct {
string
}

var envRegex = regexp.MustCompile(`^(?<key>[\w\d_]+)=(?<value>.*)$`)

func (l cronLine) exportEnv() (map[string]string, error) {
match := envRegex.FindStringSubmatch(l.string)
answer := make(map[string]string)
switch len(match) {
case 0:
case 3:
answer[match[1]] = match[2]
default:
return nil, fmt.Errorf("unexpected response from environment parser for line:\n%s", l.string)
}
if len(answer) != 1 && len(strings.Trim(l.string, " \n\t")) != 0 {
return nil, fmt.Errorf("line cannot be parsed as environment:\n%s", l.string)
}
return answer, nil
}

func (l cronLine) exportSpec(regex *regexp.Regexp, env map[string]string, parser cronSpecParser) (*cronSpec, error) {
match := regex.FindStringSubmatch(l.string)
if len(match) < 1 {
if len(strings.Trim(l.string, " \n\t")) == 0 {
return nil, nil
} else {
return nil, fmt.Errorf("cannot parse this non-empty line as cron specification: %s", l.string)
}
}
return parser(match, env), nil
}
53 changes: 53 additions & 0 deletions cmd/parser/cron_line_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package parser

import (
"testing"

"github.com/alecthomas/assert/v2"
)

func TestExportEnv_SingleMatch(t *testing.T) {
tests := []struct {
name string
input string
expected map[string]string
wantErr bool
}{
{
name: "Single match",
input: "MY_VAR=value",
expected: map[string]string{"MY_VAR": "value"},
wantErr: false,
},
{
name: "No match",
input: "no_match",
expected: nil,
wantErr: true,
},
{
name: "Empty input",
input: "",
expected: map[string]string{},
wantErr: false,
},
{
name: "Special characters",
input: "VAR_WITH_UNDERSCORE=value_with_underscore",
expected: map[string]string{"VAR_WITH_UNDERSCORE": "value_with_underscore"},
wantErr: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cl := cronLine{string: tt.input}
got, err := cl.exportEnv()
if tt.wantErr {
assert.Error(t, err)
} else {
assert.Equal(t, tt.expected, got)
}
})
}
}
50 changes: 50 additions & 0 deletions cmd/parser/cron_spec.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package parser

import (
"fmt"
"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, error) {
cronIndex := regex.SubexpIndex("cron")
// userIndex := regex.SubexpIndex("user")
cmdIndex := regex.SubexpIndex("cmd")
if cronIndex < 0 || cmdIndex < 0 {
return nil, fmt.Errorf("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,
}
}, nil
}

func withUserParser(regex *regexp.Regexp) (cronSpecParser, error) {
cronIndex := regex.SubexpIndex("cron")
userIndex := regex.SubexpIndex("user")
cmdIndex := regex.SubexpIndex("cmd")
if cronIndex < 0 || cmdIndex < 0 || userIndex < 0 {
return nil, fmt.Errorf("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,
}
}, nil
}
167 changes: 167 additions & 0 deletions cmd/parser/cron_string.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
package parser

import (
"fmt"
"regexp"
"strings"

"github.com/sirupsen/logrus"

"github.com/FMotalleb/crontab-go/config"
)

type CronString struct {
string
}

func NewCronString(cron string) CronString {
return CronString{cron}
}
FMotalleb marked this conversation as resolved.
Show resolved Hide resolved

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
Copy link

Choose a reason for hiding this comment

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

Add unit tests for replaceAll.

The function replaceAll is not covered by unit tests. It's crucial to add comprehensive unit tests for this function to ensure its reliability and correctness.

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)
	}
}
Tools
GitHub Check: codecov/patch

[warning] 21-24: cmd/parser/cron_string.go#L21-L24
Added lines #L21 - L24 were not covered by tests

}

func (s CronString) sanitizeLineBreaker() CronString {
return s.replaceAll(
`\s*\\\s*\n\s*([\n|\n\s])*`,
" ",
)
Comment on lines +27 to +31
Copy link

Choose a reason for hiding this comment

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

Add unit tests for sanitizeLineBreaker.

The function sanitizeLineBreaker is not covered by unit tests. It's crucial to add comprehensive unit tests for this function to ensure its reliability and correctness.

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)
	}
}
Tools
GitHub Check: codecov/patch

[warning] 27-31: cmd/parser/cron_string.go#L27-L31
Added lines #L27 - L31 were not covered by tests

}

func (s CronString) sanitizeEmptyLine() CronString {
return s.replaceAll(
`\n\s*\n`,
"\n",
)
Comment on lines +34 to +38
Copy link

Choose a reason for hiding this comment

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

Add unit tests for sanitizeEmptyLine.

The function sanitizeEmptyLine is not covered by unit tests. It's crucial to add comprehensive unit tests for this function to ensure its reliability and correctness.

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)
	}
}
Tools
GitHub Check: codecov/patch

[warning] 34-38: cmd/parser/cron_string.go#L34-L38
Added lines #L34 - L38 were not covered by tests

}

func (s CronString) sanitizeComments() CronString {
return s.replaceAll(
`\s*#.*`,
"",
)
Comment on lines +41 to +45
Copy link

Choose a reason for hiding this comment

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

Add unit tests for sanitizeComments.

The function sanitizeComments is not covered by unit tests. It's crucial to add comprehensive unit tests for this function to ensure its reliability and correctness.

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)
	}
}
Tools
GitHub Check: codecov/patch

[warning] 41-45: cmd/parser/cron_string.go#L41-L45
Added lines #L41 - L45 were not covered by tests

}

func (s CronString) sanitize() CronString {
return s.
replaceAll("\r\n", "\n").
sanitizeComments().
sanitizeLineBreaker().
sanitizeEmptyLine()
Copy link

Choose a reason for hiding this comment

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

Add unit tests for sanitize.

The function sanitize is not covered by unit tests. It's crucial to add comprehensive unit tests for this function to ensure its reliability and correctness.

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)
	}
}
Tools
GitHub Check: codecov/patch

[warning] 48-53: cmd/parser/cron_string.go#L48-L53
Added lines #L48 - L53 were not covered by tests

}

func (s CronString) lines() []string {
return strings.Split(s.string, "\n")
Comment on lines +65 to +66
Copy link

Choose a reason for hiding this comment

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

Add unit tests for lines.

The function lines is not covered by unit tests. It's crucial to add comprehensive unit tests for this function to ensure its reliability and correctness.

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)
		}
	}
}
Tools
GitHub Check: codecov/patch

[warning] 65-66: cmd/parser/cron_string.go#L65-L66
Added lines #L65 - L66 were not covered by tests

}

func (s *CronString) parseAsSpec(
pattern string,
hasUser bool,
) ([]cronSpec, error) {
envTable := make(map[string]string)
specs := make([]cronSpec, 0)
lines := s.sanitize().lines()
matcher, parser, err := buildMapper(hasUser, pattern)
if err != nil {
return []cronSpec{}, err
}
for _, line := range lines {
l := cronLine{line}
if env, err := l.exportEnv(); len(env) > 0 {
if err != nil {
return nil, err
}
for key, val := range env {
if old, ok := envTable[key]; ok {
logrus.Warnf("env var of key `%s`, value `%s`, is going to be replaced by `%s`", key, old, val)
}
envTable[key] = val

}
} else {
spec, err := l.exportSpec(matcher, envTable, parser)
if err != nil {
return nil, err
}
if spec != nil {
specs = append(specs, *spec)
}

}
}
return specs, nil
}
Copy link

Choose a reason for hiding this comment

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

Improve error handling and logging in parseAsSpec.

The function parseAsSpec handles errors but does not log them, which could make debugging difficult. Consider adding logging before returning errors to provide more context about the failure.

if err != nil {
+	logrus.Errorf("Error building mapper: %v", err)
	return []cronSpec{}, err
}
...
if err != nil {
+	logrus.Errorf("Error exporting spec: %v", err)
	return nil, err
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *CronString) parseAsSpec(
pattern string,
hasUser bool,
) ([]cronSpec, error) {
envTable := make(map[string]string)
specs := make([]cronSpec, 0)
lines := s.sanitize().lines()
matcher, parser, err := buildMapper(hasUser, pattern)
if err != nil {
return []cronSpec{}, err
}
for _, line := range lines {
l := cronLine{line}
if env, err := l.exportEnv(); len(env) > 0 {
if err != nil {
return nil, err
}
for key, val := range env {
if old, ok := envTable[key]; ok {
logrus.Warnf("env var of key `%s`, value `%s`, is going to be replaced by `%s`", key, old, val)
}
envTable[key] = val
}
} else {
spec, err := l.exportSpec(matcher, envTable, parser)
if err != nil {
return nil, err
}
if spec != nil {
specs = append(specs, *spec)
}
}
}
return specs, nil
}
func (s *CronString) parseAsSpec(
pattern string,
hasUser bool,
) ([]cronSpec, error) {
envTable := make(map[string]string)
specs := make([]cronSpec, 0)
lines := s.sanitize().lines()
matcher, parser, err := buildMapper(hasUser, pattern)
if err != nil {
logrus.Errorf("Error building mapper: %v", err)
return []cronSpec{}, err
}
for _, line := range lines {
l := cronLine{line}
if env, err := l.exportEnv(); len(env) > 0 {
if err != nil {
return nil, err
}
for key, val := range env {
if old, ok := envTable[key]; ok {
logrus.Warnf("env var of key `%s`, value `%s`, is going to be replaced by `%s`", key, old, val)
}
envTable[key] = val
}
} else {
spec, err := l.exportSpec(matcher, envTable, parser)
if err != nil {
logrus.Errorf("Error exporting spec: %v", err)
return nil, err
}
if spec != nil {
specs = append(specs, *spec)
}
}
}
return specs, nil
}
Tools
golangci-lint

83-83: unnecessary trailing newline

(whitespace)


93-93: unnecessary trailing newline

(whitespace)


func (s *CronString) ParseConfig(
pattern string,
hasUser bool,
) (*config.Config, error) {
specs, err := s.parseAsSpec(pattern, hasUser)
if err != nil {
return nil, err
}
cfg := &config.Config{}
for _, spec := range specs {
addSpec(cfg, spec)
}
return cfg, nil
}
Comment on lines +107 to +120
Copy link

Choose a reason for hiding this comment

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

Validate configuration before returning in ParseConfig.

The method ParseConfig constructs a configuration but does not perform any validation on the resulting configuration. It's a good practice to validate the configuration to catch any potential issues before the configuration is used elsewhere.

cfg := &config.Config{}
...
return cfg, nil
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
+	return cfg, nil

Additionally, add unit tests for ParseConfig to ensure its reliability and correctness.

func TestParseConfig(t *testing.T) {
	cron := NewCronString("0 0 * * * /command")
	cfg, err := cron.ParseConfig("* * * * *", false)
	if err != nil {
		t.Errorf("Unexpected error: %v", err)
	}
	if len(cfg.Jobs) != 1 {
		t.Errorf("Expected 1 job, got %d", len(cfg.Jobs))
	}
}
Tools
GitHub Check: codecov/patch

[warning] 110-113: cmd/parser/cron_string.go#L110-L113
Added lines #L110 - L113 were not covered by tests


[warning] 115-117: cmd/parser/cron_string.go#L115-L117
Added lines #L115 - L117 were not covered by tests


[warning] 119-119: cmd/parser/cron_string.go#L119
Added line #L119 was not covered by tests


func buildMapper(hasUser bool, pattern string) (*regexp.Regexp, cronSpecParser, error) {
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 {
return nil, nil, fmt.Errorf("cannot parse cron `%s`", matcher)
}
var parser cronSpecParser
if hasUser {
parser, err = withUserParser(matcher)
} else {
parser, err = normalParser(matcher)
}
if err != nil {
return nil, nil, err
}
return matcher, parser, nil
Copy link

Choose a reason for hiding this comment

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

Add unit tests for buildMapper.

The function buildMapper is not covered by unit tests. It's crucial to add comprehensive unit tests for this function to ensure its reliability and correctness.

func TestBuildMapper(t *testing.T) {
	matcher, parser, err := buildMapper(false, "* * * * *")
	if err != nil {
		t.Errorf("Unexpected error: %v", err)
	}
	if matcher == nil || parser == nil {
		t.Error("Expected matcher and parser to be non-nil")
	}
}
Tools
GitHub Check: codecov/patch

[warning] 111-114: cmd/parser/cron_string.go#L111-L114
Added lines #L111 - L114 were not covered by tests


[warning] 116-116: cmd/parser/cron_string.go#L116
Added line #L116 was not covered by tests


[warning] 118-120: cmd/parser/cron_string.go#L118-L120
Added lines #L118 - L120 were not covered by tests

}

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)
}
27 changes: 27 additions & 0 deletions cmd/parser/helper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package parser

import (
"encoding/json"
"fmt"

"gopkg.in/yaml.v3"

"github.com/FMotalleb/crontab-go/config"
)

func generateYamlFromCfg(finalConfig *config.Config) (string, error) {
str, err := json.Marshal(finalConfig)
if err != nil {
return "", fmt.Errorf("failed to marshal(json) final config: %v", err)
}
hashMap := make(map[string]any)
if err := json.Unmarshal(str, &hashMap); err != nil {
return "", fmt.Errorf("failed to unmarshal(json) final config: %v", err)
}
ans, err := yaml.Marshal(hashMap)
if err != nil {
return "", fmt.Errorf("failed to marshal(yaml) final config: %v", err)
}
result := string(ans)
return result, nil
}
Comment on lines +12 to +27
Copy link

Choose a reason for hiding this comment

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

Consider direct YAML serialization instead of using JSON as an intermediate format.

The function generateYamlFromCfg uses JSON marshaling and unmarshaling as intermediate steps before converting to YAML. This could lead to inefficiencies or potential data loss if the data types in config.Config do not map cleanly to JSON. Consider using direct YAML marshaling if possible to avoid these issues.

- str, err := json.Marshal(finalConfig)
- if err != nil {
-     return "", fmt.Errorf("failed to marshal(json) final config: %v", err)
- }
- hashMap := make(map[string]any)
- if err := json.Unmarshal(str, &hashMap); err != nil {
-     return "", fmt.Errorf("failed to unmarshal(json) final config: %v", err)
- }
- ans, err := yaml.Marshal(hashMap)
+ ans, err := yaml.Marshal(finalConfig)
if err != nil {
    return "", fmt.Errorf("failed to marshal(yaml) final config: %v", err)
}
result := string(ans)
return result, nil
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func generateYamlFromCfg(finalConfig *config.Config) (string, error) {
str, err := json.Marshal(finalConfig)
if err != nil {
return "", fmt.Errorf("failed to marshal(json) final config: %v", err)
}
hashMap := make(map[string]any)
if err := json.Unmarshal(str, &hashMap); err != nil {
return "", fmt.Errorf("failed to unmarshal(json) final config: %v", err)
}
ans, err := yaml.Marshal(hashMap)
if err != nil {
return "", fmt.Errorf("failed to marshal(yaml) final config: %v", err)
}
result := string(ans)
return result, nil
}
func generateYamlFromCfg(finalConfig *config.Config) (string, error) {
ans, err := yaml.Marshal(finalConfig)
if err != nil {
return "", fmt.Errorf("failed to marshal(yaml) final config: %v", err)
}
result := string(ans)
return result, nil
}

Loading