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.

Sign up for GitHub

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

Jump to bottom

Added exec.NewCommandExecutor to execute commands with correct interpreter #1075

Merged
merged 14 commits into from
Dec 21, 2023
Merged

Added exec.NewCommandExecutor to execute commands with correct interpreter #1075

merged 14 commits into from
Dec 21, 2023

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Dec 18, 2023 •
edited
Loading

Changes

Instead of handling command chaining ourselves, we execute passed commands as-is by storing them, in temp file and passing to correct interpreter (bash or cmd) based on OS.

Fixes #1065

Tests

Added unit tests

@andrewnester andrewnester requested a review from pietern December 18, 2023 17:19
@andrewnester
Copy link
Contributor Author

Ran integration tests manually: successfully passed

pietern
pietern reviewed Dec 20, 2023
View reviewed changes
libs/cmdio/exec.go Outdated
}

func findCmdExecutable(command string) (*interpreter, error) {
// Lookup for CMD executable (Windows)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment if this automatically takes care of the .exe suffix?

libs/cmdio/exec_test.go Outdated
assert.NoError(t, err)
assert.NotEmpty(t, executable)

e := NewCommandExecutor(".")
Copy link
Contributor

Choose a reason for hiding this comment

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

This test may very well use bash.

If there is a way to force the interpreter to be used (even by poking at internal fields), then we can get coverage for the cmd.exe execution paths here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pietern this test uses soecifc executable (in this case cmd) found few lines above, so it tests different interpreters correctly

pietern
pietern reviewed Dec 20, 2023
View reviewed changes
@andrewnester andrewnester requested a review from pietern December 21, 2023 11:33
pietern
pietern reviewed Dec 21, 2023
View reviewed changes
libs/exec/exec.go Outdated
}

func (e *Executor) StartCommand(ctx context.Context, command string) (Command, error) {
e.interpreter.prepare(command)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only stateful part; if someone uses the same executor to run 2 commands concurrently, then the first one to terminate will clean up the file created by the last one to start. If this is pulled out of the interpreter state and onto the stack then it looks safe. Can be done as a follow-up, as long as we document it is not safe to run 2 commands concurrently.

@pietern
Copy link
Contributor

pietern commented Dec 21, 2023

Please also update the title/description before merging.

@andrewnester andrewnester changed the title Added cmdio.NewCommandExecutor to execute artifact build commands Added exec.NewCommandExecutor to execute commands with correct interpreter Dec 21, 2023
@andrewnester andrewnester requested a review from pietern December 21, 2023 14:59
pietern
pietern approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Looks great!

Final thoughts:

  • Can use a couple more comments on the the exported structs/interfaces.
  • We should consider a different package name to disambiguate from os/exec.

libs/exec/exec_test.go
assert.NoFileExists(t, fileName)
}

func TestMultipleCommandsRunInParrallel(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func TestMultipleCommandsRunInParrallel(t *testing.T) {
func TestMultipleCommandsRunInParallel(t *testing.T) {

libs/exec/exec_test.go
assert.NoError(t, err)
}

wg.Wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice -- did this fail without the channel changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pietern yes, it was failing with file closed / file not exists error

@andrewnester andrewnester added this pull request to the merge queue Dec 21, 2023
Merged via the queue into main with commit ac37a59 Dec 21, 2023
@andrewnester andrewnester deleted the space-in-python-path branch December 21, 2023 15:50
@pietern pietern mentioned this pull request Dec 21, 2023
Closed
pietern added a commit that referenced this pull request Jan 11, 2024
@pietern
Release v0.212.0
6dbcca8 
CLI:
 * Tweak command groups in CLI help ([#1092](#1092)).
 * Fixed storage-credentials list command in text output ([#1094](#1094)).
 * Allow account client auth with environment variables when no `.databrickscfg` file present ([#1097](#1097)).
 * Always log with text format by default ([#1096](#1096)).

Bundles:
 * Added exec.NewCommandExecutor to execute commands with correct interpreter ([#1075](#1075)).
 * Use resource key as name in permissions code ([#1087](#1087)).
 * Add support for reprompts if user input does not match template schema ([#946](#946)).
 * Improve experience for multiple builtin templates ([#1052](#1052)).
 * Improve error when bundle root is not writable ([#1093](#1093)).
 * Change recommended production deployment path from `/Shared` to `/Users` ([#1091](#1091)).
 * Allow referencing bundle resources by name ([#872](#872)).
 * Make libs/exec fallback to `sh` if `bash` cannot be found ([#1114](#1114)).

Internal:
 * Functionality to walk a `config.Value` tree ([#1081](#1081)).
 * Rename libs/config -> libs/dyn ([#1086](#1086)).
 * Added codecov integration on code push ([#1095](#1095)).
 * Consolidate functions to convert `dyn.Value` to native types ([#1100](#1100)).
 * Define constant for the invalid `dyn.Value` ([#1101](#1101)).

API Changes:
 * Changed `databricks serving-endpoints update-config` command with new required argument order.
 * Changed `databricks account ip-access-lists create` command with new required argument order.
 * Changed `databricks account ip-access-lists replace` command with new required argument order.
 * Changed `databricks account ip-access-lists update` command with new required argument order.
 * Changed `databricks ip-access-lists create` command with new required argument order.
 * Changed `databricks ip-access-lists replace` command with new required argument order.
 * Changed `databricks ip-access-lists update` command with new required argument order.

OpenAPI commit a7a9dc025bb80303e676bf3708942c6aa06689f1 (2024-01-04)

Dependency updates:
 * Bump github.com/hashicorp/terraform-exec from 0.19.0 to 0.20.0 ([#1088](#1088)).
 * Bump go.uber.org/mock from 0.3.0 to 0.4.0 ([#1089](#1089)).
 * Bump github.com/hashicorp/terraform-json from 0.18.0 to 0.20.0 ([#1090](#1090)).
 * Bump golang.org/x/term from 0.15.0 to 0.16.0 ([#1103](#1103)).
 * Bump github.com/cloudflare/circl from 1.3.3 to 1.3.7 ([#1105](#1105)).
 * Bump golang.org/x/sync from 0.5.0 to 0.6.0 ([#1104](#1104)).
 * Upgrade golang.org/x/crypto to v0.17.0 in internal module ([#1110](#1110)).
 * Upgrade Go SDK to v0.29.0 ([#1111](#1111)).
@pietern pietern mentioned this pull request Jan 11, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 11, 2024
@pietern
Release v0.212.0 (#1116)
51c64ef 
CLI:
* Tweak command groups in CLI help
([#1092](#1092)).
* Fixed storage-credentials list command in text output
([#1094](#1094)).
* Allow account client auth with environment variables when no
`.databrickscfg` file present
([#1097](#1097)).
* Always log with text format by default
([#1096](#1096)).

Bundles:
* Added exec.NewCommandExecutor to execute commands with correct
interpreter ([#1075](#1075)).
* Use resource key as name in permissions code
([#1087](#1087)).
* Add support for reprompts if user input does not match template schema
([#946](#946)).
* Improve experience for multiple builtin templates
([#1052](#1052)).
* Improve error when bundle root is not writable
([#1093](#1093)).
* Change recommended production deployment path from `/Shared` to
`/Users` ([#1091](#1091)).
* Allow referencing bundle resources by name
([#872](#872)).
* Make libs/exec fallback to `sh` if `bash` cannot be found
([#1114](#1114)).

Internal:
* Functionality to walk a `config.Value` tree
([#1081](#1081)).
* Rename libs/config -> libs/dyn
([#1086](#1086)).
* Added codecov integration on code push
([#1095](#1095)).
* Consolidate functions to convert `dyn.Value` to native types
([#1100](#1100)).
* Define constant for the invalid `dyn.Value`
([#1101](#1101)).

API Changes:
* Changed `databricks serving-endpoints update-config` command with new
required argument order.
* Changed `databricks account ip-access-lists create` command with new
required argument order.
* Changed `databricks account ip-access-lists replace` command with new
required argument order.
* Changed `databricks account ip-access-lists update` command with new
required argument order.
* Changed `databricks ip-access-lists create` command with new required
argument order.
* Changed `databricks ip-access-lists replace` command with new required
argument order.
* Changed `databricks ip-access-lists update` command with new required
argument order.

OpenAPI commit a7a9dc025bb80303e676bf3708942c6aa06689f1 (2024-01-04)

Dependency updates:
* Bump github.com/hashicorp/terraform-exec from 0.19.0 to 0.20.0
([#1088](#1088)).
* Bump go.uber.org/mock from 0.3.0 to 0.4.0
([#1089](#1089)).
* Bump github.com/hashicorp/terraform-json from 0.18.0 to 0.20.0
([#1090](#1090)).
* Bump golang.org/x/term from 0.15.0 to 0.16.0
([#1103](#1103)).
* Bump github.com/cloudflare/circl from 1.3.3 to 1.3.7
([#1105](#1105)).
* Bump golang.org/x/sync from 0.5.0 to 0.6.0
([#1104](#1104)).
* Upgrade golang.org/x/crypto to v0.17.0 in internal module
([#1110](#1110)).
* Upgrade Go SDK to v0.29.0
([#1111](#1111)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Databricks bundle deploy fails when python.exe/python3.exe path has a space on windows
2 participants
@andrewnester @pietern