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

[ISSUE #674] feat: Add support for env variables in executor config #677

Closed
wants to merge 5 commits into from

Conversation

halalala222
Copy link
Contributor

export env

export DAGU_SSH_CONFIG_PASSWORD="password"
export DAGU_SSH_CONFIG_IP="ip"
export DAGU_SSH_CONFIG_USER="user"
export DAGU_SSH_CONFIG_PORT="22"

func newSSHExec(_ context.Context, step dag.Step) (Executor, error) {
// expend env
if err = expendExecConfigEnv(step.ExecutorConfig.Config); err != nil {
return nil, err
}

if err = md.Decode(step.ExecutorConfig.Config); err != nil {
	return nil, err
}

}
75d2aff794bde8915dd250eb16684a0
74cd5158848c2808e1149058db0257e

@yohamta
Copy link
Collaborator

yohamta commented Sep 5, 2024

Hi @halalala222, thank you very much for your great work! After reviewing the code, I noticed that it's replacing specific environment variables. I apologize for not being clear about this earlier, but we need to expand all environment variables included in the Config. For example, it should work like this:

steps:
  - name: step1
    executor:
      type: ssh
      config:
        user: "${SSH_USER}"
        ip: "${SSH_HOST}"
        port: "${SSH_PORT}"
        key: "${HOME}/.ssh/id_rsa"
    command: /usr/sbin/ifconfig

I think we can achieve this using the following approach:

// expandEnvHook is a mapstructure decode hook that expands environment variables in string fields
func expandEnvHook(f reflect.Type, t reflect.Type, data interface{}) (interface{}, error) {
	if f.Kind() != reflect.String || t.Kind() != reflect.String {
		return data, nil
	}
	return os.ExpandEnv(data.(string)), nil
}

func newSSHExec(_ context.Context, step dag.Step) (Executor, error) {
	cfg := new(sshExecConfig)
	decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
		Result: cfg,
		DecodeHook: expandEnvHook,
	})
	// ... omitted ...
}

Additionally, if possible, it would be great if you could add some tests for this functionality. Thank you very much for your help. It's really appreciated!

@halalala222
Copy link
Contributor Author

Hi ! @yohamta Thank you ! I know ! I will try to work on it.

@yohamta
Copy link
Collaborator

yohamta commented Sep 11, 2024

Hi @halalala222, I understand you might be busy right now. Would you mind if I take over the task? If you'd prefer to continue working on it, that's great too.

@halalala222
Copy link
Contributor Author

Hi @yohamta ,I'm really sorry, but I have some matters to attend to recently and may not be able to cover this task anymore. I apologize for the inconvenience.

@yohamta
Copy link
Collaborator

yohamta commented Sep 11, 2024

Hi @halalala222, no worries at all! Thank you so much for all the wonderful work you've done on developing Dagu, it was incredibly helpful. I'm closing this for now.

@yohamta yohamta closed this Sep 11, 2024
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.68%. Comparing base (3c4f636) to head (b20c3ab).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #677   +/-   ##
=======================================
  Coverage   64.68%   64.68%           
=======================================
  Files          53       53           
  Lines        4315     4315           
=======================================
  Hits         2791     2791           
  Misses       1269     1269           
  Partials      255      255           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c4f636...b20c3ab. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants