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

Add Built-in Execution Context Variables #581

Open
yohamta opened this issue Jun 1, 2024 · 7 comments
Open

Add Built-in Execution Context Variables #581

yohamta opened this issue Jun 1, 2024 · 7 comments
Labels
enhancement New feature to be implemented good first issue Good for newcomers

Comments

@yohamta
Copy link
Collaborator

yohamta commented Jun 1, 2024

Overview
Introduce built-in execution context variables for each DAG execution.

Variables

  • DAG_SCHEDULER_LOG_PATH
  • DAG_EXECUTION_LOG_PATH
  • DAG_REQUEST_ID

Example Usage

handlerOn:
  failure:
    command: "echo There was an error while doing ${DAG_REQUEST_ID} request"

Requirements

  • Extend the DAGContext struct to hold necessary values
  • Modify executors to pass required environment variables when running their commands
  • Update documents (optional)

Additional Context

@yohamta yohamta added good first issue Good for newcomers enhancement New feature to be implemented labels Jun 1, 2024
@yohamta yohamta added this to the 1.13.1 milestone Jun 1, 2024
@yohamta yohamta removed this from the 1.13 milestone Jul 15, 2024
@halalala222
Copy link
Contributor

@yohamta
hi , can i try to work on this issue ?

@yohamta
Copy link
Collaborator Author

yohamta commented Aug 6, 2024

Hi @halalala222 , thank you for your interest! That would be greatly helpful. Please don't hesitate to ask me know if you have any questions or need clarification on anything.

@halalala222
Copy link
Contributor

halalala222 commented Aug 7, 2024

@yohamta I have found some problems,when i work on this issue
In this issue requirements :
image
in /dag/scheduler/node.go
image
uitil.SplitCommandWithParse() function
image
the os.ExpandEnv() Will replace the string ${DAG_REQUEST_ID} with the corresponding environment variable.
so Modify executors to pass required environment variables when running their commands unable to implement the corresponding functionality.
Below are some methods I have considered for implementation:

  1. When ${val} starts with 'dagu' or others indicating Built-in Execution Context Variables, skip the os.ExpandEnv(unescapeReplacer.Replace(v)). After that in executors can pass required environment variables when running their commands.
  2. Use $(val) instead of ${val}, but it cannot rely on environment variables. Use strings.replaceAll to substitute.

I would like to ask if there is any better solution?

@halalala222
Copy link
Contributor

@yohamta If the main scenario for this feature is to print the corresponding variables through a command execution, should other executors besides command be added?

@yohamta
Copy link
Collaborator Author

yohamta commented Aug 7, 2024

Hi @halalala222, thank you for the detailed explanation and those great solution ideas. This is very helpful. It is a problem that I was not aware of 😅 A possible solution that came to my mind is to replace the ${DAG_SCHEDULER_LOG_PATH} to some intermediate expression, such as {{DAG_SCHEDULER_LOG_PATH}} during the DAG building stage, and replace back to ${DAG_SCHEDULER_LOG_PATH} when node setup is called. What do you think?

@halalala222
Copy link
Contributor

Hi @yohamta
I believe this is an excellent solution! I'll start working on it. Thank you! Your help has been invaluable.

@halalala222
Copy link
Contributor

#654 Hi @yohamta , in this pr i try to work on it. I hope u can take a look at my PR. Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature to be implemented good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants