Skip to content

chore: CATALOG-11541 Use catalog changes in fork repo #1

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

Merged
merged 4 commits into from
May 29, 2025

Conversation

BC-RSikora
Copy link

@BC-RSikora BC-RSikora commented May 26, 2025

Jira: CATALOG-11541

What/Why?

Use catalog changes in fork repo.
Cherry-picked all commits from previous repo: https://github.com/bc-ochkovskyi/laravel-queue-rabbitmq

Rollout/Rollback

Revert PR.

Testing

By unit tests:
Screenshot 2025-05-29 at 13 19 51

In cat-imex service: https://github.com/bigcommerce/catalog-import-export/pull/568
In orchestrator-imex service: https://github.com/bigcommerce/import-export-service/pull/173

@BC-RSikora BC-RSikora requested a review from a team May 26, 2025 11:29
@BC-RSikora BC-RSikora force-pushed the CATALOG-11541 branch 3 times, most recently from aaeb848 to 290b04e Compare May 26, 2025 17:17
@BC-RSikora BC-RSikora marked this pull request as ready for review May 28, 2025 11:49
Comment on lines 104 to 116
protected static function getReadWriteTimeoutFromConfig(AMQPConnectionConfig $connectionConfig, array $config): void
{
$readTimeout = Arr::get($config, self::CONFIG_OPTIONS.'.read_timeout');
$writeTimeout = Arr::get($config, self::CONFIG_OPTIONS.'.write_timeout');

if (is_numeric($readTimeout) && intval($readTimeout) > 0) {
$connectionConfig->setReadTimeout((int) $readTimeout);
}

if (is_numeric($writeTimeout) && intval($writeTimeout) > 0) {
$connectionConfig->setWriteTimeout((int) $readTimeout);
}
}

Choose a reason for hiding this comment

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

If I get right, this config is the only reason of switching to fork. Isn't it?
Is it possible to use custom connection class in sake of using original vendor?

Copy link
Author

Choose a reason for hiding this comment

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

I tried do that in previous ticket but unsuccessful because of changing Connection config class instead of Connection class. And we decided to use fork for that.

$readTimeout = Arr::get($config, self::CONFIG_OPTIONS.'.read_timeout');
$writeTimeout = Arr::get($config, self::CONFIG_OPTIONS.'.write_timeout');

if (is_numeric($readTimeout) && intval($readTimeout) > 0) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (is_numeric($readTimeout) && intval($readTimeout) > 0) {
$readTtl = is_numeric($readTimeout) ? (int)readTimeout : 0;
if ($readTtl > 0) {

🍹 cast only once.

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -99,4 +100,18 @@ protected static function getNetworkProtocolFromConfig(AMQPConnectionConfig $con
$connectionConfig->setNetworkProtocol($networkProtocol);
}
}

protected static function getReadWriteTimeoutFromConfig(AMQPConnectionConfig $connectionConfig, array $config): void
Copy link

Choose a reason for hiding this comment

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

could you add tests 🙏

Copy link
Author

Choose a reason for hiding this comment

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

added

@BC-RSikora BC-RSikora merged commit 3f65041 into master May 29, 2025
@BC-RSikora BC-RSikora deleted the CATALOG-11541 branch May 29, 2025 14:15
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.

4 participants