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

Allow authentication for nats connections #4412

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Dec 15, 2023

Allows authenticating nats connections with username/password

@kobergj kobergj marked this pull request as ready for review December 15, 2023 14:44
@kobergj kobergj requested review from a team, labkode and glpatcern as code owners December 15, 2023 14:44
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

one remark. I'd build the stream.NatsConfig beforhand and apply only the values that are set. But since this is just following the way the other properties are set I'm ok with merging this as is.

Comment on lines 237 to 238
AuthUsername: m["username"].(string),
AuthPassword: m["password"].(string),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a little bit fragile ... if the config struct m does not have a key set this will panic with

"interface conversion: interface {} is nil, not string"

same for all the other keys, here ... still kinda meh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍 I moved the definition to the source struct and use mapstructure.Decode to unmarshal it. Please double check.

@butonic butonic merged commit da04bc3 into cs3org:edge Dec 19, 2023
9 checks passed
@micbar micbar mentioned this pull request Dec 20, 2023
71 tasks
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.

None yet

3 participants