Merged
Conversation
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
requested review from
andrewnester,
anton-107,
denik and
pietern
as code owners
Merged
pietern
reviewed
May 13, 2025
pietern
approved these changes
May 13, 2025
Contributor
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
The PR description talks about env var interpolation, but more importantly, we would need to perform lexical analysis to split a single string into args if we don't use a shell.
Env var interpolation in the script contents is more a side effect than a feature we explicitly need, correct?
Comments are not blocking for merge.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Author
|
@pietern We do want environment variable interpolation since otherwise users will not have a way to configure inputs to their scripts. Good point about the lexical analysis, modified the PR description. |
pietern
reviewed
May 14, 2025
|
|
||
| // Variant of [Execv] that runs the given script through a shell | ||
| func ShellExecv(content, dir string, env []string) error { | ||
| newOpts, err := shellExecvOpts(content, dir, env) |
Contributor
There was a problem hiding this comment.
Nit: new in newOpts is no longer accurate
This was referenced May 14, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 6, 2025
## Why This part of the codebase was missing test coverage. I plan on changing the execution of commands to inline the script content and these tests are required to ensure I don't break anything (relevant discussion: #2862 (comment)) --------- Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 13, 2025
## Changes This PR inlines the script content using the `-c` flag in bash and sh instead of passing them in via `-e <file>`. ## Why Based on discussion in #2862 (comment). Getting rid of the temp file means we no longer have to clean up the temporary file created. ## Tests Tests that were added in: #2884
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
ShellExecv is a variant of the Execv method that also configures a shell to be used before running the command.
Why
We need it for the scripts section in DABs: #2813.
Why:
Note: This uses the same priority order for shells as the
artifacts.buildfield, that is bash > sh > powershell.Tests
Unit test. Also acceptance tests in #2813