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.

Sign up for GitHub

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: removing duplicated nuget packages folder #746

Merged
merged 4 commits into from
Jan 28, 2025
Merged

fix: removing duplicated nuget packages folder #746

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0ada78
fix: removing duplicated nuget packages folder
Jan 24, 2025
bcd66a9
fix: removing unnecessary logging
Jan 26, 2025
0f9056e
fix: updating logging message
Jan 27, 2025
f122265
fix: updating copyFile operation to be sync to confirm that all files…
Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions server/aws-lsp-codewhisperer/src/language-server/netTransform/artifactManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Logging, Workspace } from '@aws/language-server-runtimes/server-interface'
import * as archiver from 'archiver'
import * as crypto from 'crypto'

Check warning on line 3 in server/aws-lsp-codewhisperer/src/language-server/netTransform/artifactManager.ts

View workflow job for this annotation

GitHub Actions / Test 

Do not import Node.js builtin module "crypto"

Check warning on line 3 in server/aws-lsp-codewhisperer/src/language-server/netTransform/artifactManager.ts

View workflow job for this annotation

GitHub Actions / Test (Windows) 

Do not import Node.js builtin module "crypto"
import * as fs from 'fs'

Check warning on line 4 in server/aws-lsp-codewhisperer/src/language-server/netTransform/artifactManager.ts

View workflow job for this annotation

GitHub Actions / Test 

Do not import Node.js builtin module "fs"

Check warning on line 4 in server/aws-lsp-codewhisperer/src/language-server/netTransform/artifactManager.ts

View workflow job for this annotation

GitHub Actions / Test (Windows) 

Do not import Node.js builtin module "fs"
import { CodeFile, Project, References, RequirementJson, StartTransformRequest } from './models'
import path = require('path')
const requriementJsonFileName = 'requirement.json'
Expand All @@ -9,6 +9,7 @@
const referencesFolderName = 'references'
const zipFileName = 'artifact.zip'
const sourceCodeFolderName = 'sourceCode'
const packagesFolderName = 'packages'

export class ArtifactManager {
private workspace: Workspace
Expand All @@ -22,6 +23,7 @@
async createZip(request: StartTransformRequest): Promise<string> {
await this.createRequirementJson(request)
await this.copySolutionConfigFiles(request)
await this.removeDuplicateNugetPackagesFolder(request)
return await this.zipArtifact()
}
async removeDir(dir: string) {
Expand All @@ -41,6 +43,21 @@
}
}

async removeDuplicateNugetPackagesFolder(request: StartTransformRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as a potential optimization, is it possible to skip adding these files to the zip in the first place. If I understand correctly right now we spend extra time copying these files in the step before only to end up going through the folder again and deleting them. If we could create the artifacts in a single go that would also make the process more efficient. Is this possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes Ege, thanks for suggestion.
After this hotfix, we will take a stab in detail on optimizing entire zipping with ticket P194692593.

const packagesFolder = path.join(
this.workspacePath,
artifactFolderName,
sourceCodeFolderName,
packagesFolderName
)
if (!fs.existsSync(packagesFolder)) {
this.logging.log('Cannot find nuget packages folder in source code')
Copy link

Choose a reason for hiding this comment

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

Why do we need to log this?

Copy link

@jonlouie jonlouie Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

+1. If we invert the if logic, we can simplify this method. Then reducing the logging becomes natural.

Copy link
Contributor Author

@pranav-firake pranav-firake Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

agreed, it is causing unnecessary logs, will remove this.
will add just return here instead of logging

return
}
fs.rmSync(packagesFolder, { recursive: true, force: true })
this.logging.log(`Removed nuget packages folder: ${packagesFolder}`)
}

async createRequirementJson(request: StartTransformRequest) {
const fileContent = await this.createRequirementJsonContent(request)
const dir = this.getRequirementJsonPath()
Expand Down Expand Up @@ -194,6 +211,10 @@
this.createFolderIfNotExist(dir)
fs.copyFile(sourceFilePath, destFilePath, error => {
if (error) {
if (!fs.existsSync(dir) && dir.includes(packagesFolderName)) {
//Packages folder has been deleted to avoid duplicates in artifacts.zip
return
}
this.logging.log('Failed to copy: ' + sourceFilePath + error)
}
})
Expand Down
Loading