Skip to content

Fix bundle plan to not create workspace objects or upload the files#3442

Merged
andrewnester merged 17 commits intomainfrom
fix/bundle-plan-upload
Sep 1, 2025
Merged

Fix bundle plan to not create workspace objects or upload the files#3442
andrewnester merged 17 commits intomainfrom
fix/bundle-plan-upload

Conversation

@andrewnester
Copy link
Copy Markdown
Contributor

Changes

Refactored deployPrepare to make it non-mutative. Moved artifact folder cleanup and libraries upload to uploadLibraries, which is only used by deploy.

Why

This makes bundle plan non-mutative and safe to use to preview bundle changes.

Tests

Added acceptance test

terraform.CheckRunningResource(),

// artifacts.CleanUp() is there because I'm not sure if it's safe to move to later stage.
artifacts.CleanUp(),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We call CleanUp later now, but before doing libraries.Upload. This appears to be safe because we do not rely on the remote artifacts folder anywhere before we call libraries.Upload(),

@eng-dev-ecosystem-bot
Copy link
Copy Markdown
Collaborator

eng-dev-ecosystem-bot commented Aug 19, 2025

Run: 17376336627

Env ✅​pass 🙈​skip
✅​ aws linux 308 510
✅​ aws windows 309 509
✅​ aws-ucws linux 420 408
✅​ aws-ucws windows 421 407
✅​ azure linux 308 509
✅​ azure windows 309 508
✅​ azure-ucws linux 420 407
✅​ azure-ucws windows 421 406
✅​ gcp linux 307 511
✅​ gcp windows 308 510

@andrewnester andrewnester requested a review from denik August 25, 2025 11:46
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2025
)

## Changes
Convert artifacts_test and upload_test into acceptance tests

## Why
Needed for #3442

<!-- If your PR needs to be included in the release notes for next
release,
add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
@@ -0,0 +1,20 @@
bundle:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For more comprehensive test, can we add "bundle plan" + request recording to default-python various local and integration tests and check that there are no side effects there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried to include bundle plan in default-python template tests but it appeared not trivial as these are quite complex tests and adding bundle plan added additional output just as bundle.tf.json and terraform.lock files. Will follow up with these tests afterwards not to block this PR.

}

uploadPath := path.Join(b.Config.Workspace.ArtifactPath, InternalDirName)
uploadPath = ensureWorkspaceOrVolumesPrefix(uploadPath)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a comment why this is needed? Unclear why we would be missing prefix now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added, it's not that we miss it now, I just moved it from the other place to here, but I added the comment to the function

@andrewnester andrewnester added this pull request to the merge queue Sep 1, 2025
Merged via the queue into main with commit 429bedc Sep 1, 2025
13 checks passed
@andrewnester andrewnester deleted the fix/bundle-plan-upload branch September 1, 2025 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants