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.

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

fix: Refactoring the download process for StaticSiteClient #936

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shibayan
Copy link
Contributor

@shibayan shibayan commented Mar 3, 2025

Fixed an issue where ETXTBSY and EBUSY errors would occur when executing processes using spawn after the download process ran when StaticSiteClient did not exist.

In this PR, we simplified the close processing and error handling after downloading the file by using the Streams Promises API, and stabilized the download process.

I wrote the following workflow and checked the failure rate of the swa deploy command in the branches before and after the improvement.

jobs:
  deploy:
    runs-on: windows-latest
    steps:
    - uses: actions/checkout@v4
    
    - uses: actions/checkout@v4
      with:
        repository: shibayan/static-web-apps-cli
        ref: deploy-client-download
        path: static-web-apps-cli

    - name: Build SWA CLI
      run: |
        npm install
        npm run build
      working-directory: ./static-web-apps-cli

    - name: Deploy to Static Web App
      run: node ./static-web-apps-cli/dist/cli/bin.js deploy ./dist/ --env production

deploy-new.yml is the workflow that uses the fixes in this PR. I can see that the deployment stability has improved significantly, as the failure rate of the workflow after the fixes is 0%, whereas the failure rate of the workflow before the fixes was over 60%.

image

image

Fixes #721
Fixes #799
Fixes #916

@github-actions github-actions bot added the scope: core Issues happened a the ./src/core level label Mar 3, 2025
@shibayan
Copy link
Contributor Author

shibayan commented Mar 4, 2025

@Timothyw0 @cjk7989 I consider this to be a fairly critical issue, so it would be very helpful if you could review it 🙏 Thanks!

Copy link

@mohamedfarees mohamedfarees left a comment

Choose a reason for hiding this comment

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

Based on test result, LGTM, It will be great if this fix goes to live

@dvasdekis
Copy link

@Timothyw0 and @cjk7989 are you able to review this pull request? We are an Azure Enterprise customer and this is creating issues for us. Happy to include our account manager if that would expedite it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: core Issues happened a the ./src/core level
Projects
None yet
3 participants