Skip to content

fix: novadev-630#145

Closed
dlilly wants to merge 0 commit into
amplience:masterfrom
dlilly:master
Closed

fix: novadev-630#145
dlilly wants to merge 0 commit into
amplience:masterfrom
dlilly:master

Conversation

@dlilly
Copy link
Copy Markdown
Contributor

@dlilly dlilly commented Sep 2, 2022

No description provided.

Copy link
Copy Markdown
Contributor

@rs-amp rs-amp left a comment

Choose a reason for hiding this comment

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

Manually performing auth via requests in the new rest client is unnecessary, you can use the existing OAuth2Client provided by dc-management-sdk. There are no unit tests for the rate-publish-queue. There was an existing version of the existing publish queue that supported parallel publishes and had unit tests, it was removed in a commit a while back, but there weren't actually many changes from the existing one:

8aa6f9b

You could use this to implement some of the unit tests as they should be similar.

I don't know why the build is failing, but the lock file updating to 2 is a bit suspect. You should develop dc-cli on a version before the lockfile change, as we publish to older node versions that don't support it.

Comment thread src/commands/content-item/import.ts Outdated
if (argv.publish) {
const pubQueue = new PublishQueue(argv);
const pubQueue = new RateLimitedPublishQueue(argv);
// const pubQueue = new PublishQueue(argv);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Either remove this comment (and the PublishQueue class) or make it optional via an argument.

Comment thread src/common/import/amplience-rest-client.ts Outdated
const itemStart = new Date().valueOf();
const result = await this.publishContentItem(item);

console.log(`Publish ${result.item.label} = ${result.state}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here. If the log is valuable, then it should be done with the LogFile class.

Comment thread src/common/import/rate-limit-publish-queue.ts Outdated
rs-amp added a commit that referenced this pull request Sep 12, 2022
This is an alternative implementation to #145 which extends the existing publishing queue to support concurrent publishes.

Benefits:

- Publish() method actually starts the publish, dispatching is limited by both the rate limit and the concurrency limit.
- Not too much change from the existing publish queue.

Downsides:

- Custom implementation of batching rather than a standard one, though this was technically already in place originally.
@dlilly dlilly closed this Sep 12, 2022
Pureball pushed a commit to cosnova/dc-cli that referenced this pull request Aug 22, 2023
)

This is an alternative implementation to amplience#145 which extends the existing publishing queue to support concurrent publishes.

Benefits:

- Publish() method actually starts the publish, dispatching is limited by both the rate limit and the concurrency limit.
- Not too much change from the existing publish queue.

Downsides:

- Custom implementation of batching rather than a standard one, though this was technically already in place originally.
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.

2 participants