internal: Refactor immutable folder upload as an internal resource in direct engine - #6084
internal: Refactor immutable folder upload as an internal resource in direct engine#6084andrewnester wants to merge 12 commits into
Conversation
Approval status: pending
|
Integration test reportCommit: 54a7dc0
9 interesting tests: 4 RECOVERED, 4 SKIP, 1 FAIL
Top 5 slowest tests (at least 2 minutes):
|
| Deploying resources... | ||
| Updating deployment state... | ||
| Deployment complete! | ||
| \nSnapshot did not change as expected |
There was a problem hiding this comment.
nit: \n is printed verbatim rather than as newline.
There was a problem hiding this comment.
Also, is this expected "Snapshot did not change as expected"?
| "deployments": [ | ||
| { | ||
| "command_path": "${workspace.snapshot_path}/files/src/main.py", | ||
| "command_path": "${internal.snapshot.full_path}/files/src/main.py", |
There was a problem hiding this comment.
Instead of custom path, can we keep using ${workspace.file_path}, just make it an alias internally to internal resource's snapshot path.
That way:
- users know what this means
- users can use the variable themselves and it'll work both in regular and immutable case.
There was a problem hiding this comment.
${workspace.file_path} is ${internal.snapshot.full_path}/files and because we resolve the variables ${workspace.file_path} resolved to this pass. I'd prefer not to change the way variable reslution works and introduce some custom aliases
| } | ||
|
|
||
| func (v *filesToSync) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { | ||
| // Immutable folder does not use the sync mechanism, so we don't need to validate it. |
There was a problem hiding this comment.
why not? This validates "sync:" section in the config, is that ignored for immutable deployments?
I thought the source set of files is unchanged for immutable deployments, is that not true?
| @@ -35,12 +36,15 @@ type ACLEntry struct { | |||
| // the same workspace location. | |||
| // This interface exists so the implementation can later be replaced with a Go SDK call. | |||
There was a problem hiding this comment.
This does not explain why we need the interface - do we plan on supporting multiple implementation at the same time?
| trace $CLI jobs get $JOB_ID | jq '.settings.tasks' | jq '.[] | select(.spark_python_task != null) | .spark_python_task.python_file' | ||
| trace $CLI jobs get $JOB_ID | jq '.settings.tasks' | jq '.[] | select(.notebook_task != null) | .notebook_task.notebook_path' | ||
| trace $CLI jobs get $JOB_ID | jq '.settings.tasks' | jq '.[] | select(.notebook_task != null) | .notebook_task.base_parameters.path' | ||
| PYTHON_FILE_1=$($CLI jobs get $JOB_ID | jq '.settings.tasks' | jq '.[] | select(.spark_python_task != null) | .spark_python_task.python_file') |
There was a problem hiding this comment.
we can store "jobs get" output and query it multiple times - would matter if we run this test on Cloud.
| trace jq '.plan["resources.snapshots.immutable"]' plan.json | ||
| rm plan.json | ||
|
|
||
| trace $CLI bundle deploy |
There was a problem hiding this comment.
could you read EnvMatrix.READPLAN here for serialized plan? This is good at catching issues with implicit state transfer.
| Deploying resources... | ||
| Updating deployment state... | ||
| Deployment complete! | ||
| \nSnapshot did not change as expected |
There was a problem hiding this comment.
Also, is this expected "Snapshot did not change as expected"?
3 participants
Changes
Refactor immutable folder upload as an internal resource in direct engine
Why
This enables us:
${internal.snapshot.full_path}and it's automatically resolved during deployTests
Existing tests pass