Add acceptance test for `bundle run` #2695

shreyas-goenka · 2025-04-09T16:23:12Z

Followup to #2413, adding more coverage to ensure we have proper disambiguation between when bundle run is used to run a resource vs execute a script.

denik

I'd also add a map storing runId -> jobs.Run so that runs/get can return an error when wrong run is requested (this is the level of support we have for all other resources).

This reverts commit 4c327f5.

shreyas-goenka · 2025-05-09T13:42:33Z

@denik Made the fake server stateful, can you take another look?

denik · 2025-05-09T13:59:07Z

bundle/run/job.go

@@ -188,7 +188,7 @@ func (r *jobRunner) Run(ctx context.Context, opts *Options) (output.RunOutput, e

 	waiter, err := w.Jobs.RunNow(ctx, *req)
 	if err != nil {
-		return nil, fmt.Errorf("cannot start job: %w", err)
+		return nil, errors.New("cannot start job")


What's the motivation for this? Why have less details?

Just splitting it into a separate PR: #2843 since it's not directly related. This PR does not remove it.

denik · 2025-05-09T13:59:29Z

cmd/bundle/run.go

@@ -104,9 +104,6 @@ task or a Python wheel task, the second example applies.
 You can also use the bundle run command to execute scripts / commands in the same
 authentication context as the bundle.

-Note: The current working directory of the provided command will be set to the root
-of the bundle.


Is this not true anymore? why this message is deleted?

I don't see this diff, are you reviewing a specific commit?

denik · 2025-05-09T14:02:51Z

libs/testserver/fake_workspace.go

+	}
+
+	// Mark the run as terminated.
+	run.State.LifeCycleState = jobs.RunLifeCycleStateTerminated


This seems to be very specific to a particular test, might not be reusable. Perhaps instead you can have explicit endpoint to update this field?

The bundle run command blocks for the run to terminate. The run state is an internal detail and all runs automatically transition to terminated, so it's a fair way to model this in a fake workspace.

OK, we can extend this later.

denik · 2025-05-09T14:06:06Z

acceptance/bundle/run/basic/script

@@ -0,0 +1,16 @@
+title "no run key specified"
+errcode trace $CLI bundle run


nit: personally prefer smaller tests, easier to review/debug and faster to run. e.g. you could split error-raising commands from the rest.

if you have one command then you also don't need to specify title (it's test name), errcode and trace.

Fair enough, but in this case it's only 5 commands so still small. The flip side is you end up with a bunch of really granular files / tests which does not seem optimal.

denik · 2025-05-09T14:06:52Z

.wsignore

@@ -19,6 +19,7 @@ acceptance/selftest/record_cloud/volume-io/hello.txt

 # "bundle run" has trailing whitespace:
 acceptance/bundle/integration_whl/*/output.txt
+acceptance/bundle/run/basic/output.txt


why is it here? The last command is "echo" which includes a newline.

The run command itself has a trailing space in it's output, not the echo command. Specifically:

[DATE] HH:MM:SS "run-name" TERMINATED

Is this easily fixable? I would prefer to keep this ignore list to the absolute minimum.

Trying to fix in #2864

denik

Thanks!

Addressed feedback from: #2695 (comment)

shreyas-goenka added 30 commits February 28, 2025 14:12

Add the auth.EnvVars function

31b7ad0

Add the auth.ProcessEnv function

bf234c4

-

42f6ecf

[WIP] Add bundle exec

4f43fb9

-

90c6ad0

some more tests

6002e0d

execute scripts from bundle root

1b5ff48

clarify the cwd plan

c442378

add cases for the target flag

152d982

fix replacement

f0d1dc5

make the profile is passed test work

080d1bf

add more tests for profile and target

58d28a9

added flags are not parsed check

9939562

exit code done

05cd18c

cleanup

a2748d5

merge

898b2c1

do not run on cloud

1996d3f

use $CLI instead of databricks

88b6dc8

return stdout / stderr errors after

fd2600c

simplify streaming

93c5e3f

-

bbbbf30

-

eb58d11

-

007a714

-

0c9fbf7

more cleanup

edf7051

-

769acf7

-

4061036

-

c0d34f7

-

112a9de

some cleanup

647dab0

shreyas-goenka temporarily deployed to test-trigger-is April 9, 2025 17:02 — with GitHub Actions Inactive

denik approved these changes Apr 11, 2025

View reviewed changes

Base automatically changed from bundle-exec to main April 24, 2025 21:36

shreyas-goenka added 2 commits May 9, 2025 14:31

checkout from main

fd43bd7

merge

fd2f755

shreyas-goenka temporarily deployed to test-trigger-is May 9, 2025 12:32 — with GitHub Actions Inactive

-

ff71a22

shreyas-goenka temporarily deployed to test-trigger-is May 9, 2025 12:34 — with GitHub Actions Inactive

-

576a16e

shreyas-goenka temporarily deployed to test-trigger-is May 9, 2025 12:34 — with GitHub Actions Inactive

undo fix

1f36d74

shreyas-goenka temporarily deployed to test-trigger-is May 9, 2025 12:44 — with GitHub Actions Inactive

add stateful tracking for runs

4c327f5

shreyas-goenka temporarily deployed to test-trigger-is May 9, 2025 12:55 — with GitHub Actions Inactive

shreyas-goenka added 2 commits May 9, 2025 15:31

Revert "add stateful tracking for runs"

2a06b49

This reverts commit 4c327f5.

proper stateful tracking

72305f1

shreyas-goenka temporarily deployed to test-trigger-is May 9, 2025 13:38 — with GitHub Actions Inactive

-

92bf456

shreyas-goenka temporarily deployed to test-trigger-is May 9, 2025 13:41 — with GitHub Actions Inactive

denik reviewed May 9, 2025

View reviewed changes

denik self-requested a review May 9, 2025 14:07

denik approved these changes May 9, 2025

View reviewed changes

shreyas-goenka added this pull request to the merge queue May 9, 2025

Merged via the queue into main with commit 6586291 May 9, 2025
10 checks passed

shreyas-goenka deleted the test-bundle-run branch May 9, 2025 14:40

shreyas-goenka mentioned this pull request May 13, 2025

Remove trailing whitespace from bundle run output #2864

Merged

github-merge-queue bot pushed a commit that referenced this pull request May 13, 2025

Remove trailing whitespace from bundle run output (#2864)

347426e

Addressed feedback from: #2695 (comment)

anton-107 pushed a commit that referenced this pull request May 14, 2025

Remove trailing whitespace from bundle run output (#2864)

c7a030f

Addressed feedback from: #2695 (comment)

		@@ -0,0 +1,16 @@
		title "no run key specified"
		errcode trace $CLI bundle run

Add acceptance test for bundle run #2695

Add acceptance test for bundle run #2695

Uh oh!

Conversation

shreyas-goenka commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

denik left a comment

Choose a reason for hiding this comment

Uh oh!

shreyas-goenka commented May 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyas-goenka May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Add acceptance test for `bundle run` #2695

Add acceptance test for `bundle run` #2695

shreyas-goenka commented Apr 9, 2025 •

edited

Loading

shreyas-goenka May 9, 2025 •

edited

Loading