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: cmd arg mode #41

Merged
merged 8 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
12 changes: 9 additions & 3 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@ LOG_FILE=/var/log/crontab-go.log
LOG_STDOUT=true
LOG_LEVEL=debug

SHELL=/bin/bash
SHELL_ARGS=-c

WEBSERVER_ADDRESS=
WEBSERVER_PORT=
WEBSERVER_USERNAME=admin
WEBSERVER_PASSWORD=f2f9899c-567c-455f-8a82-77a2c66e736e
WEBSERVER_METRICS=true

TZ=Asia/Tehran

# defaults to sh on linux and cmd on windows
SHELL=/bin/bash
# can be an array (`:` seperated) the seperation can be escaped using `\:`
SHELL_ARGS=-c

# env value will enable the `CRONTAB_GO_EVENT_ARGUMENTS` environments variable and event data will be passed using this env
# Can be [none (default), env, arg] any other value will not break the program but will be oprational as default value
SHELL_ARG_COMPATIBILITY=none
5 changes: 4 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,24 @@ jobs:
run: make ci
- name: Upload coverage
uses: actions/upload-artifact@v4
if: ${{ github.event_name != 'pull_request' }}
with:
name: coverage-${{ matrix.os }}
path: coverage.*

- run: goreleaser release --rm-dist --snapshot
if: ${{ runner.os == 'Linux' }}
if: ${{ runner.os == 'Linux' && github.event_name != 'pull_request' }}

- name: Upload dist
uses: actions/upload-artifact@v4
if: ${{ github.event_name != 'pull_request' }}
with:
name: dist-${{ matrix.os }}
path: dist

- name: Upload coverage to Codecov
uses: codecov/codecov-action@v4
if: ${{ github.event_name != 'pull_request' }}
with:
fail_ci_if_error: true
file: ./coverage.out
Expand Down
2 changes: 1 addition & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
linters-settings:
funlen:
lines: 100
lines: 150
statements: 50
gocyclo:
min-complexity: 15
Expand Down
8 changes: 7 additions & 1 deletion cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func Execute() {
}

func init() {
warnOnErr(godotenv.Load(), "Cannot initialize .env file: %s")
_ = godotenv.Load()

rootCmd.AddCommand(parser.ParserCmd)
rootCmd.PersistentFlags().StringVarP(&cfgFile, "config", "c", "", "config file (default is config.yaml)")
Expand Down Expand Up @@ -188,6 +188,12 @@ func setupEnv() {
),
"Cannot bind shell_args env variable: %s",
)
warnOnErr(
viper.BindEnv(
"shell_arg_compatibility",
),
"Cannot bind shell_arg_compatibility env variable: %s",
)
Comment on lines +191 to +196
Copy link

Choose a reason for hiding this comment

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

LGTM, but add tests.

The code changes to add a new binding for the shell_arg_compatibility environment variable are approved.

However, the added lines are not covered by tests according to the codecov report.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 191-196: cmd/root.go#L191-L196
Added lines #L191 - L196 were not covered by tests


viper.AutomaticEnv()
}
2 changes: 1 addition & 1 deletion config.local.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
jobs:
- name: echo
tasks:
- command: echo "$0 $@"
- command: echo "$CRONTAB_GO_EVENT_ARGUMENTS"
env:
"COLE": test
events:
Expand Down
11 changes: 11 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ type Config struct {
WebServerPassword string `mapstructure:"webserver_password" json:"webserver_password,omitempty"`
WebServerMetrics bool `mapstructure:"webserver_metrics" json:"webserver_metrics,omitempty"`

ShellArgCompatibility ShellArgCompatibilityMode `mapstructure:"shell_arg_compatibility" json:"shell_arg_compatibility,omitempty"`

Jobs []*JobConfig `mapstructure:"jobs" json:"jobs"`
}

Expand Down Expand Up @@ -109,3 +111,12 @@ type TaskConnection struct {
Volumes []string `mapstructure:"volumes" json:"volumes,omitempty"`
Networks []string `mapstructure:"networks" json:"networks,omitempty"`
}

type ShellArgCompatibilityMode string

const (
DefaultShellArgCompatibility ShellArgCompatibilityMode = EventArgOmit
EventArgOmit ShellArgCompatibilityMode = "none"
EventArgPassingAsArgs ShellArgCompatibilityMode = "arg"
EventArgPassingAsEnviron ShellArgCompatibilityMode = "env"
)
144 changes: 144 additions & 0 deletions core/cmd_connection/cmd_utils/cmd_context.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
// Package cmdutils contain helper methods for cmd executors
package cmdutils

import (
"context"
"fmt"
"os"
"strings"

"github.com/sirupsen/logrus"

"github.com/FMotalleb/crontab-go/config"
"github.com/FMotalleb/crontab-go/core/utils"
"github.com/FMotalleb/crontab-go/ctxutils"
)

type Ctx struct {
context.Context
logger *logrus.Entry
}

func NewCtx(
ctx context.Context,
taskEnviron map[string]string,
logger *logrus.Entry,
) *Ctx {
result := &Ctx{
Context: ctx,
logger: logger,
}
result.init(taskEnviron)
return result
}

func (ctx *Ctx) init(taskEnviron map[string]string) {
osEnviron := os.Environ()
ctx.logger.Trace("Initial environment variables: ", osEnviron)
ctx.Context = context.WithValue(
ctx,
ctxutils.Environments,
map[string]string{},
)
for _, pair := range osEnviron {
parts := strings.SplitN(pair, "=", 2)
if len(parts) == 2 {
ctx.envAdd(parts[0], parts[1])
}
}
for key, val := range taskEnviron {
ctx.envAdd(key, val)
switch strings.ToLower(key) {
case "shell":
ctx.logger.Info("you've used `SHELL` env variable in command environments, overriding the global shell with:", val)
case "shell_args":
ctx.logger.Info("you've used `SHELL_ARGS` env variable in command environments, overriding the global shell_args with: ", val)
case "shell_arg_compatibility":
ctx.logger.Info("you've used `SHELL_ARG_COMPATIBILITY` env variable in command environments, overriding the global shell_arg_compatibility with: ", val)
}
}
}

func (ctx *Ctx) envGetAll() map[string]string {
if env := ctx.Value(ctxutils.Environments); env != nil {
return env.(map[string]string)
}
return map[string]string{}
}

func (ctx *Ctx) envGet(key string) string {
return ctx.envGetAll()[key]
}

func (ctx *Ctx) envAdd(key string, value string) {
oldEnv := ctx.envGetAll()
key = strings.ToUpper(key)
oldEnv[key] = value
ctx.Context = context.WithValue(
ctx,
ctxutils.Environments,
oldEnv,
)
}

func (ctx *Ctx) envReshape() []string {
env := ctx.envGetAll()
var result []string
for key, val := range env {
result = append(result, fmt.Sprintf("%s=%s", strings.ToUpper(key), val))
}
return result
}

func (ctx *Ctx) getShell() string {
return ctx.envGet("SHELL")
}

func (ctx *Ctx) getShellArg() string {
return ctx.envGet("SHELL_ARGS")
}

func (ctx *Ctx) getShellArgCompatibility() config.ShellArgCompatibilityMode {
result := config.ShellArgCompatibilityMode(ctx.envGet("SHELL_ARG_COMPATIBILITY"))
switch result {
case "":
return config.DefaultShellArgCompatibility
default:
return result
}
}

func (ctx *Ctx) BuildExecuteParams(command string, eventData []string) (shell string, cmd []string, env []string) {
environments := ctx.envReshape()
shell = ctx.getShell()
shellArgs := utils.EscapedSplit(ctx.getShellArg(), ':')
shellArgs = append(shellArgs, command)
switch ctx.getShellArgCompatibility() {
case config.EventArgOmit:
ctx.logger.Debug("event arguments will not be passed to the command")
case config.EventArgPassingAsArgs:
shellArgs = append(shellArgs, eventData...)
case config.EventArgPassingAsEnviron:
environments = append(
environments,
fmt.Sprintf("CRONTAB_GO_EVENT_ARGUMENTS=%s",
collectEventForEnv(eventData),
),
)
default:
ctx.logger.Warn("event argument passing mode is not supported, using default mode (omitting)")
}
return shell, shellArgs, environments
}

func collectEventForEnv(eventData []string) string {
builder := &strings.Builder{}
for i, part := range eventData {
builder.WriteString(strings.ReplaceAll(part, ":", "\\:"))
if i < len(eventData)-1 {
builder.WriteRune(':')
}
}

return builder.String()
}
Comment on lines +1 to +144
Copy link

Choose a reason for hiding this comment

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

LGTM, but add tests.

The core/cmd_connection/cmd_utils/cmd_context.go file is approved as it provides a clean and modular way to manage context and environment variables for command execution. The Ctx struct and its methods encapsulate the logic for managing environment variables and building command execution parameters, making the code more readable and maintainable. The BuildExecuteParams method handles the different shell argument compatibility modes, allowing for flexibility in how event data is passed to the command. The collectEventForEnv function ensures that colons in the event data are properly escaped when passing them as an environment variable.

However, many lines in the file are not covered by tests according to the codecov report.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 26-29: core/cmd_connection/cmd_utils/cmd_context.go#L26-L29
Added lines #L26 - L29 were not covered by tests


[warning] 31-32: core/cmd_connection/cmd_utils/cmd_context.go#L31-L32
Added lines #L31 - L32 were not covered by tests


[warning] 35-46: core/cmd_connection/cmd_utils/cmd_context.go#L35-L46
Added lines #L35 - L46 were not covered by tests


[warning] 49-57: core/cmd_connection/cmd_utils/cmd_context.go#L49-L57
Added lines #L49 - L57 were not covered by tests


[warning] 62-64: core/cmd_connection/cmd_utils/cmd_context.go#L62-L64
Added lines #L62 - L64 were not covered by tests


[warning] 66-66: core/cmd_connection/cmd_utils/cmd_context.go#L66
Added line #L66 was not covered by tests


[warning] 69-70: core/cmd_connection/cmd_utils/cmd_context.go#L69-L70
Added lines #L69 - L70 were not covered by tests


[warning] 73-81: core/cmd_connection/cmd_utils/cmd_context.go#L73-L81
Added lines #L73 - L81 were not covered by tests


[warning] 84-88: core/cmd_connection/cmd_utils/cmd_context.go#L84-L88
Added lines #L84 - L88 were not covered by tests


[warning] 90-90: core/cmd_connection/cmd_utils/cmd_context.go#L90
Added line #L90 was not covered by tests


[warning] 93-94: core/cmd_connection/cmd_utils/cmd_context.go#L93-L94
Added lines #L93 - L94 were not covered by tests


[warning] 97-98: core/cmd_connection/cmd_utils/cmd_context.go#L97-L98
Added lines #L97 - L98 were not covered by tests


[warning] 101-107: core/cmd_connection/cmd_utils/cmd_context.go#L101-L107
Added lines #L101 - L107 were not covered by tests


[warning] 111-129: core/cmd_connection/cmd_utils/cmd_context.go#L111-L129
Added lines #L111 - L129 were not covered by tests


[warning] 131-131: core/cmd_connection/cmd_utils/cmd_context.go#L131
Added line #L131 was not covered by tests


[warning] 134-139: core/cmd_connection/cmd_utils/cmd_context.go#L134-L139
Added lines #L134 - L139 were not covered by tests


[warning] 143-143: core/cmd_connection/cmd_utils/cmd_context.go#L143
Added line #L143 was not covered by tests

14 changes: 5 additions & 9 deletions core/cmd_connection/docker_attach.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/FMotalleb/crontab-go/abstraction"
"github.com/FMotalleb/crontab-go/config"
cmdutils "github.com/FMotalleb/crontab-go/core/cmd_connection/cmd_utils"
"github.com/FMotalleb/crontab-go/ctxutils"
)

Expand Down Expand Up @@ -50,7 +51,7 @@ func NewDockerAttachConnection(log *logrus.Entry, conn *config.TaskConnection) a
// Returns:
// - An error if the preparation fails, otherwise nil.
func (d *DockerAttachConnection) Prepare(ctx context.Context, task *config.Task) error {
shell, shellArgs, env := reshapeEnviron(task.Env, d.log)
cmdCtx := cmdutils.NewCtx(ctx, task.Env, d.log)
Copy link

Choose a reason for hiding this comment

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

LGTM, but add tests.

The code change to use the cmdutils.NewCtx function is approved as it improves the clarity and modularity of the code.

However, the added line is not covered by tests according to the codecov report.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 54-54: core/cmd_connection/docker_attach.go#L54
Added line #L54 was not covered by tests

d.ctx = ctx
// Specify the container ID or name
d.containerID = d.conn.ContainerName
Expand All @@ -59,22 +60,17 @@ func (d *DockerAttachConnection) Prepare(ctx context.Context, task *config.Task)
d.conn.DockerConnection = "unix:///var/run/docker.sock"
}
params := ctx.Value(ctxutils.EventData).([]string)
shell, shellArgs, environments := cmdCtx.BuildExecuteParams(task.Command, params)
Copy link

Choose a reason for hiding this comment

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

LGTM, but add tests.

The code change to use the BuildExecuteParams method from the cmdCtx object is approved as it streamlines the process of preparing the command to be executed within the Docker container and leads to a cleaner and more maintainable code structure.

However, the added line is not covered by tests according to the codecov report.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 63-63: core/cmd_connection/docker_attach.go#L63
Added line #L63 was not covered by tests

cmd := append(
[]string{shell},
append(
shellArgs,
append(
[]string{task.Command},
params...,
)...,
)...,
shellArgs...,
Copy link

Choose a reason for hiding this comment

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

LGTM, but add tests.

The code change to append the shellArgs variable to the cmd slice is approved as it is a minor refactoring and does not affect the functionality.

However, the added line is not covered by tests according to the codecov report.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 66-66: core/cmd_connection/docker_attach.go#L66
Added line #L66 was not covered by tests

)
// Create an exec configuration
d.execCFG = &container.ExecOptions{
AttachStdout: true,
AttachStderr: true,
Privileged: true,
Env: env,
Env: environments,
Copy link

Choose a reason for hiding this comment

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

LGTM, but add tests.

The code change to set the Env field of the execCFG struct to the environments variable derived from the cmdCtx object is approved as it updates the environment variable handling to utilize the new environments variable, replacing the earlier env variable.

However, the added line is not covered by tests according to the codecov report.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 73-73: core/cmd_connection/docker_attach.go#L73
Added line #L73 was not covered by tests

WorkingDir: task.WorkingDirectory,
User: task.UserName,
Cmd: cmd,
Expand Down
18 changes: 7 additions & 11 deletions core/cmd_connection/docker_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"context"
"io"
"strings"

"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/network"
Expand All @@ -13,6 +12,8 @@ import (

"github.com/FMotalleb/crontab-go/abstraction"
"github.com/FMotalleb/crontab-go/config"
cmdutils "github.com/FMotalleb/crontab-go/core/cmd_connection/cmd_utils"
"github.com/FMotalleb/crontab-go/core/utils"
"github.com/FMotalleb/crontab-go/ctxutils"
"github.com/FMotalleb/crontab-go/helpers"
)
Expand Down Expand Up @@ -53,34 +54,29 @@ func NewDockerCreateConnection(log *logrus.Entry, conn *config.TaskConnection) a
// Returns:
// - An error if the preparation fails, otherwise nil.
func (d *DockerCreateConnection) Prepare(ctx context.Context, task *config.Task) error {
shell, shellArgs, env := reshapeEnviron(task.Env, d.log)
cmdCtx := cmdutils.NewCtx(ctx, task.Env, d.log)
d.ctx = ctx
if d.conn.DockerConnection == "" {
d.log.Debug("No explicit docker connection specified, using default: `unix:///var/run/docker.sock`")
d.conn.DockerConnection = "unix:///var/run/docker.sock"
}

params := ctx.Value(ctxutils.EventData).([]string)
shell, shellArgs, environments := cmdCtx.BuildExecuteParams(task.Command, params)
cmd := append(
[]string{shell},
append(
shellArgs,
append(
[]string{task.Command},
params...,
)...,
)...,
shellArgs...,
)
volumes := make(map[string]struct{})
for _, volume := range d.conn.Volumes {
inContainer := strings.Split(volume, ":")[1]
inContainer := utils.EscapedSplit(volume, ':')[1]
volumes[inContainer] = struct{}{}
}
// Create an exec configuration
d.containerConfig = &container.Config{
AttachStdout: true,
AttachStderr: true,
Env: env,
Env: environments,
Comment on lines +57 to +79
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The Prepare method lacks direct unit test coverage.

The search results did not reveal any unit tests specifically targeting the Prepare method of the DockerCreateConnection struct. It is recommended to add unit tests to ensure this method's functionality is thoroughly tested.

  • Consider adding unit tests for the Prepare method to verify its behavior and ensure robustness.
Analysis chain

LGTM, but ensure the changes are thoroughly tested.

The changes to the Prepare method streamline the command preparation process and improve the maintainability of the code by leveraging utility functions and encapsulating context-related data.

Run the following script to verify the changes:

The static analysis tool indicates that the changes are not covered by tests. Do you want me to add unit tests for the Prepare method or open a GitHub issue to track this task?

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the `Prepare` method changes are thoroughly tested.

# Test: Search for the `Prepare` method. Expect: Corresponding unit tests.
ast-grep --lang go --pattern $'func (d *DockerCreateConnection) Prepare($_, $_) error {
  $$$
}'

Length of output: 3328


Script:

#!/bin/bash
# Description: Search for test files or functions that reference the `Prepare` method or the `DockerCreateConnection` struct.

# Find all Go test files.
fd --extension go --exec rg 'DockerCreateConnection|Prepare' {}

Length of output: 1360

Tools
GitHub Check: codecov/patch

[warning] 57-57: core/cmd_connection/docker_create.go#L57
Added line #L57 was not covered by tests


[warning] 65-65: core/cmd_connection/docker_create.go#L65
Added line #L65 was not covered by tests


[warning] 68-68: core/cmd_connection/docker_create.go#L68
Added line #L68 was not covered by tests


[warning] 72-72: core/cmd_connection/docker_create.go#L72
Added line #L72 was not covered by tests


[warning] 79-79: core/cmd_connection/docker_create.go#L79
Added line #L79 was not covered by tests

WorkingDir: task.WorkingDirectory,
User: task.UserName,
Cmd: cmd,
Expand Down
Loading
Loading