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

feat(microservices): add type for rmq connection options #13105

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

Deniks
Copy link
Contributor

@Deniks Deniks commented Jan 27, 2024

change socketOptions.connectionOptions type from any to amqp-manager's AmqpConnectionOptions which allows to specify not only mTLS but also External auth

Closes #13071

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Currently in RabbitMq client options socketOptions.connectionOptions type is any
Issue Number: #13071

What is the new behavior?

  • RabbitMq client options socketOptions.connectionOptions type is now AmqpConnectionOptions
  • In server-rmq.ts and client-rmq.ts local variable rqmPackage has been renamed to rmqPackage

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

coveralls commented Jan 27, 2024

Pull Request Test Coverage Report for Build 99c0b257-fa5c-4adb-8791-cd5ea5b9e55c

  • -2 of 6 (66.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.197%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/client/client-rmq.ts 2 3 66.67%
packages/microservices/server/server-rmq.ts 2 3 66.67%
Totals Coverage Status
Change from base Build 889ffab4-7a4b-4e81-8f8a-630e303f4274: 0.0%
Covered Lines: 6723
Relevant Lines: 7292

💛 - Coveralls

@@ -25,7 +27,7 @@ export interface AmqpConnectionManagerSocketOptions {
reconnectTimeInSeconds?: number;
heartbeatIntervalInSeconds?: number;
findServers?: () => string | string[];
connectionOptions?: any;
connectionOptions?: AmqpConnectionManager['connectionOptions'];
Copy link
Member

Choose a reason for hiding this comment

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

we can't directly reference types from the amqp-connection-manager package as this would lead to compilation errors for those who have "skipLibCheck" set to false in their TS configuration; that's why we have the external directory with copied interfaces

Copy link
Contributor Author

@Deniks Deniks Jan 29, 2024

Choose a reason for hiding this comment

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

Understood. Was there a reason for setting connectionOptions to any and not further copy AmqpConnectionOptions?
If not, I can similiarly copy the interface. As per #13071, copying just AmqpConnectionOptions.credentials field technically will suffice and allow to specify External auth, or should I specify whole interface? I think whole interface because ts will complain if user wants to specify other field than credentials.

Copy link
Member

Choose a reason for hiding this comment

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

I can't recall any specific reason so feel free to copy it over

change socketOptions.connectionOptions type from any to copied amqp-manager's AmqpConnectionOptions

Closes nestjs#13071
@Deniks Deniks force-pushed the deniks-rmq-options-fix branch from d637bd4 to 4008cc2 Compare January 31, 2024 23:04
/**
* @publicApi
*/
export interface AmqpConnectionManagerSocketOptions {
reconnectTimeInSeconds?: number;
heartbeatIntervalInSeconds?: number;
findServers?: () => string | string[];
connectionOptions?: any;
connectionOptions?: AmqpConnectionOptions;
clientProperties?: ClientProperties;
Copy link
Contributor Author

@Deniks Deniks Jan 31, 2024

Choose a reason for hiding this comment

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

a bit wierd that clientProperties in original amqp manager is inside connectionOptions .
I suspect interfaces were not updated when amqp-connection-manager version got changed?

@kamilmysliwiec kamilmysliwiec merged commit 7786a65 into nestjs:master Feb 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RabbitMQ connection options
3 participants