PR comments

Signed-off-by: Vidhan Shah <vidhans958@gmail.com>
This commit is contained in:
Vidhan Shah
2025-11-21 20:54:32 +05:30
parent 87da464220
commit e6314f2534
2 changed files with 44 additions and 31 deletions
@@ -38,6 +38,8 @@ import request from 'supertest';
import path from 'path';
import fs from 'fs-extra';
import { AwsS3Publish } from './awsS3';
jest.setTimeout(30_000);
import { Readable } from 'stream';
import {
createMockDirectory,
@@ -45,7 +47,9 @@ import {
} from '@backstage/backend-test-utils';
const env = process.env;
let s3Mock: any;
let s3Mock: ReturnType<typeof mockClient> & {
send: (command: any) => Promise<any>;
};
// Create a new MockDirectory for each test to avoid Windows file locking issues
let mockDir: ReturnType<typeof createMockDirectory>;
@@ -514,7 +518,7 @@ describe('AwsS3Publish', () => {
`default/component/backstage/assets/main.css`,
]),
});
}, 30000);
});
it('should publish a directory as well when legacy casing is used', async () => {
const publisher = await createPublisherFromConfig({
@@ -527,7 +531,7 @@ describe('AwsS3Publish', () => {
`default/Component/backstage/assets/main.css`,
]),
});
}, 30000);
});
it('should publish a directory when root path is specified', async () => {
const publisher = await createPublisherFromConfig({
@@ -540,7 +544,7 @@ describe('AwsS3Publish', () => {
`backstage-data/techdocs/default/component/backstage/assets/main.css`,
]),
});
}, 30000);
});
it('should publish a directory when root path is specified and legacy casing is used', async () => {
const publisher = await createPublisherFromConfig({
@@ -554,7 +558,7 @@ describe('AwsS3Publish', () => {
`backstage-data/techdocs/default/Component/backstage/assets/main.css`,
]),
});
}, 30000);
});
it('should publish a directory when sse is specified', async () => {
const publisher = await createPublisherFromConfig({
@@ -567,7 +571,7 @@ describe('AwsS3Publish', () => {
'default/component/backstage/assets/main.css',
]),
});
}, 30000);
});
it('should fail to publish a directory', async () => {
const wrongPathToGeneratedDirectory = mockDir.resolve(
@@ -602,7 +606,7 @@ describe('AwsS3Publish', () => {
expect(loggerInfoSpy).toHaveBeenCalledWith(
`Successfully deleted stale files for Entity ${entity.metadata.name}. Total number of files: 1`,
);
}, 30000);
});
it('should log error when the stale files deletion fails', async () => {
const bucketName = 'delete_stale_files_error';
@@ -613,7 +617,7 @@ describe('AwsS3Publish', () => {
expect(loggerErrorSpy).toHaveBeenLastCalledWith(
'Unable to delete file(s) from AWS S3. Error: Message',
);
}, 30000);
});
});
describe('hasDocsBeenGenerated', () => {
@@ -621,7 +625,7 @@ describe('AwsS3Publish', () => {
const publisher = await createPublisherFromConfig();
await publisher.publish({ entity, directory });
expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true);
}, 30000);
});
it('should return true if docs has been generated even if the legacy case is enabled', async () => {
const publisher = await createPublisherFromConfig({
@@ -629,7 +633,7 @@ describe('AwsS3Publish', () => {
});
await publisher.publish({ entity, directory });
expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true);
}, 30000);
});
it('should return true if docs has been generated if root path is specified', async () => {
const publisher = await createPublisherFromConfig({
@@ -637,7 +641,7 @@ describe('AwsS3Publish', () => {
});
await publisher.publish({ entity, directory });
expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true);
}, 30000);
});
it('should return true if docs has been generated if root path is specified and legacy casing is used', async () => {
const publisher = await createPublisherFromConfig({
@@ -646,7 +650,7 @@ describe('AwsS3Publish', () => {
});
await publisher.publish({ entity, directory });
expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true);
}, 30000);
});
it('should return false if docs has not been generated', async () => {
const publisher = await createPublisherFromConfig();
@@ -669,7 +673,7 @@ describe('AwsS3Publish', () => {
expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual(
techdocsMetadata,
);
}, 30000);
});
it('should return tech docs metadata even if the legacy case is enabled', async () => {
const publisher = await createPublisherFromConfig({
@@ -679,7 +683,7 @@ describe('AwsS3Publish', () => {
expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual(
techdocsMetadata,
);
}, 30000);
});
it('should return tech docs metadata even if root path is specified', async () => {
const publisher = await createPublisherFromConfig({
@@ -689,7 +693,7 @@ describe('AwsS3Publish', () => {
expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual(
techdocsMetadata,
);
}, 30000);
});
it('should return tech docs metadata if root path is specified and legacy casing is used', async () => {
const publisher = await createPublisherFromConfig({
@@ -700,7 +704,7 @@ describe('AwsS3Publish', () => {
expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual(
techdocsMetadata,
);
}, 30000);
});
it('should return tech docs metadata when json encoded with single quotes', async () => {
const techdocsMetadataPath = path.join(
@@ -722,7 +726,7 @@ describe('AwsS3Publish', () => {
);
fs.writeFileSync(techdocsMetadataPath, techdocsMetadataContent);
}, 30000);
});
it('should return an error if the techdocs_metadata.json file is not present', async () => {
const publisher = await createPublisherFromConfig();
@@ -776,7 +780,7 @@ describe('AwsS3Publish', () => {
const publisher = await createPublisherFromConfig();
await publisher.publish({ entity, directory });
app = express().use(publisher.docsRouter());
}, 30000);
});
it('should pass expected object path to bucket', async () => {
// Ensures leading slash is trimmed and encoded path is decoded.
@@ -173,7 +173,7 @@ export class AwsS3Publish implements PublisherBase {
'techdocs.publisher.awsS3.s3ForcePathStyle',
);
// AWS MAX ATTEMPTS is an optional config. If missing, default value of 3 is used
// AWS MAX ATTEMPTS is an optional config. If missing, default value of 5 is used
const maxAttempts = config.getOptionalNumber(
'techdocs.publisher.awsS3.maxAttempts',
);
@@ -434,11 +434,8 @@ export class AwsS3Publish implements PublisherBase {
// e.g. ['index.html', 'sub-page/index.html', 'assets/images/favicon.png']
absoluteFilesToUpload = await getFileTreeRecursively(directory);
let uploadCounter = 0;
await bulkStorageOperation(
async absoluteFilePath => {
uploadCounter++;
const relativeFilePath = path.relative(directory, absoluteFilePath);
const s3Key = getCloudPathForLocalPath(
entity,
@@ -446,10 +443,14 @@ export class AwsS3Publish implements PublisherBase {
useLegacyPathCasing,
bucketRootPath,
);
// Create params without the Body because the body must be the
// actual file contents (Buffer or Readable), not the path string.
// For multipart uploads we attach a Readable stream to avoid
// buffering large files in memory. For simple uploads we attach
// a Buffer read from disk.
const params: PutObjectCommandInput = {
Bucket: this.bucketName,
Key: s3Key,
Body: absoluteFilePath,
...(sse && { ServerSideEncryption: sse }),
};
@@ -462,15 +463,23 @@ export class AwsS3Publish implements PublisherBase {
if (fileSizeInBytes >= MAX_SINGLE_UPLOAD_BYTES) {
// Try multipart upload for large files
try {
const upload = new Upload({
client: this.storageClient,
params,
partSize: MAX_SINGLE_UPLOAD_BYTES,
queueSize: 3,
leavePartsOnError: false,
});
// Create stream and Upload inside retry closure so stream is
// recreated on each retry attempt (streams are consumable once).
await this.retryOperation(
() => upload.done(),
() => {
// Create a fresh stream on each attempt
const fileStream = fs.createReadStream(absoluteFilePath);
const uploadParams = { ...params, Body: fileStream };
const upload = new Upload({
client: this.storageClient,
params: uploadParams,
partSize: MAX_SINGLE_UPLOAD_BYTES,
queueSize: 3,
leavePartsOnError: false,
});
return upload.done();
},
`Upload-${params.Key}`,
this.maxAttempts,
);