-
Notifications
You must be signed in to change notification settings - Fork 134
improve --env handling in CLI job run #1567
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
base: main
Are you sure you want to change the base?
Conversation
Deploying datachain with
|
| Latest commit: |
5aa99e8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a5383cac.datachain-2g6.pages.dev |
| Branch Preview URL: | https://fix-cli-env-handling.datachain-2g6.pages.dev |
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.
Pull request overview
This PR improves the --env flag handling for the datachain job run command to properly support multiple environment variable declarations. Previously, when multiple --env flags were provided, only the last one would be used (silently ignoring others). The fix allows users to specify environment variables using either multiple --env flags or multiple values in a single --env flag.
Changes:
- Added
action="append"to the--envargument parser to accumulate all--envflag values - Implemented flattening logic to properly handle the resulting list of lists before joining into environment string
- Updated tests to verify both usage patterns (multiple flags and multiple values per flag)
- Enhanced documentation with examples showing both supported patterns
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/datachain/cli/parser/job.py |
Added action="append" to the --env argument to accumulate multiple flag instances |
src/datachain/studio.py |
Added flatten import and logic to properly process the nested list structure from argparse |
tests/test_cli_studio.py |
Updated test to validate both single --env with multiple values and multiple --env flags |
docs/commands/job/run.md |
Added new example demonstrating both patterns for specifying multiple environment variables and renumbered subsequent examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Tests seems to be failing, can you please take a look? |
|
@amritghimire unrelated, tocrchcodec, but yes, need to be addressed before merging ... I thought @ilongin fixed it tbh today, but it seems it something different. |
4a8e973 to
5aa99e8
Compare
dreadatour
left a comment
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.
👍
Now we can pass:
to
datachain job run- it confused me, it confused AI, etc (it was silently just using the last entry)