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

chore: bound the parallelism #162

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Nov 5, 2024

Add the linter rule that checks for bounded parallelism, and mark the 2 locations that do Promise.all as bounded.

Add the linter rule that checks for bounded parallelism,
and mark the 2 locations that do `Promise.all` as bounded.
@rix0rrr rix0rrr requested review from a team November 5, 2024 13:45
@mrgrain mrgrain changed the title chore: bound the parallelism fix: bound the parallelism Nov 5, 2024
@@ -119,7 +120,11 @@ export class AssetPublishing implements IPublishProgress {
*/
public async publish(options: PublishOptions = {}): Promise<void> {
if (this.publishInParallel) {
await Promise.all(this.assets.map(async (asset) => this.publishAsset(asset, options)));
const limit = pLimit(20);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need/want to be this restrictive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Twenty is plenty

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

Sicne this is a public facing package, can you phrase the title and description more customer focused. It will show up in the changelog (or not get released if we change it back to a chore).

@rix0rrr rix0rrr changed the title fix: bound the parallelism chore: bound the parallelism Nov 5, 2024
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Nov 5, 2024

Should have been a chore, thanks

@rix0rrr rix0rrr requested review from mrgrain and a team November 5, 2024 13:57
@mrgrain mrgrain closed this Nov 5, 2024
auto-merge was automatically disabled November 5, 2024 15:27

Pull request was closed

@mrgrain mrgrain reopened this Nov 5, 2024
@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Nov 5, 2024
Merged via the queue into main with commit b6fbdbe Nov 5, 2024
12 checks passed
@aws-cdk-automation aws-cdk-automation deleted the huijbers/bound-parallelism branch November 5, 2024 15:30
@rix0rrr rix0rrr added the backport-to-v2-main Backport this PR to the v2-main branch label Nov 6, 2024
aws-cdk-automation pushed a commit that referenced this pull request Nov 6, 2024
Add the linter rule that checks for bounded parallelism, and mark the 2
locations that do `Promise.all` as bounded.

(cherry picked from commit b6fbdbe)

# Conflicts:
#	.projen/deps.json
#	.projen/tasks.json
#	package.json
#	test/files.test.ts
@aws-cdk-automation
Copy link
Collaborator

💚 All backports created successfully

Status Branch Result
v2-main

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

github-merge-queue bot pushed a commit that referenced this pull request Nov 6, 2024
# Backport

This will backport the following commits from `main` to `v2-main`:
- [chore: bound the parallelism
(#162)](#162)

<!--- Backport version: 9.5.1 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

---------

Signed-off-by: github-actions <[email protected]>
Co-authored-by: Rico Hermans <[email protected]>
Co-authored-by: github-actions <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v2-main Backport this PR to the v2-main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants