Skip to content

Conversation

@neilk-aws
Copy link
Contributor

Problem

When zipping context for /dev, customer build processes (file watchers, etc.) may delete build artifacts we’ve already enumerated but have not added to the archive. As a best practice, customers should .gitignore these types of files, but in the event they don't, this has the potential to block /dev from running.

Solution

Skip affected files, which are not found when adding to zip context for Feature Dev.


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@neilk-aws neilk-aws requested review from a team as code owners January 6, 2025 22:17
@github-actions
Copy link

github-actions bot commented Jan 6, 2025

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@neilk-aws neilk-aws force-pushed the neilk-aws/fix-prepare-context branch from 216dd73 to 20d19cd Compare January 6, 2025 22:44
@Hweinstock
Copy link
Contributor

Is there a way we can test this behavior? Something along the lines of having collectFiles return some files that don't exist when we try to access them?

@neilk-aws neilk-aws force-pushed the neilk-aws/fix-prepare-context branch from 20d19cd to d8aff5e Compare January 7, 2025 18:58
} catch (error) {
if (error instanceof Error && error.message.includes('File not found')) {
// No-op: Skip if file was deleted or does not exist
// Reference: https://github.com/cthackers/adm-zip/blob/1cd32f7e0ad3c540142a76609bb538a5cda2292f/adm-zip.js#L296-L321
Copy link
Contributor

@justinmk3 justinmk3 Jan 7, 2025

Choose a reason for hiding this comment

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

does it set a code ? try hasCode

function hasCode<T>(error: T): error is T & { code: string } {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neilk-aws neilk-aws force-pushed the neilk-aws/fix-prepare-context branch from d8aff5e to d12d1d6 Compare January 7, 2025 20:38
@neilk-aws
Copy link
Contributor Author

Is there a way we can test this behavior? Something along the lines of having collectFiles return some files that don't exist when we try to access them?

This is mitigating a situation where there is a race condition and the file is deleted as they are being zipped. In this case, collectFiles would not report any files do not exist, and we would only discover it when they are being looked up.

@justinmk3 justinmk3 merged commit 5e1a94b into aws:master Jan 7, 2025
26 checks passed
karanA-aws pushed a commit to karanA-aws/aws-toolkit-vscode that referenced this pull request Jan 17, 2025
…#6312

## Problem
When zipping context for /dev, customer build processes (file watchers,
etc.) may delete build artifacts we’ve already enumerated but have not
added to the archive. As a best practice, customers should `.gitignore`
these types of files, but in the event they don't, this has the
potential to block /dev from running.

## Solution
Skip affected files, which are not found when adding to zip context for
Feature Dev.
s7ab059789 pushed a commit to s7ab059789/aws-toolkit-vscode that referenced this pull request Feb 19, 2025
…#6312

## Problem
When zipping context for /dev, customer build processes (file watchers,
etc.) may delete build artifacts we’ve already enumerated but have not
added to the archive. As a best practice, customers should `.gitignore`
these types of files, but in the event they don't, this has the
potential to block /dev from running.

## Solution
Skip affected files, which are not found when adding to zip context for
Feature Dev.
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