-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix for stream imports and leafnodes, fixes #1332 #1335
Conversation
Signed-off-by: Derek Collison <derek@nats.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but a question about the test.
users: [ | ||
{user: bad, password: pwd} | ||
] | ||
exports: [{stream: "foo"}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it "fun" maybe you could have made the export "bar" and then publish on "bar" and make sure it is received (on "foo"). This will make sure that the LS+ is sent on "bar" and not ">" (or "foo"), no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean remap the foo on import with a prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be on prefix + "foo" for streams. But will add that to make sure we do prefix subj.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought that we were mapping.. for stream there is only prefix..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For completeness, test a service too?
Signed-off-by: Derek Collison <derek@nats.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just saw Matthias response and already merged. I think the RI fix covers services.. |
LGTM - to be sure, will try service with my setup |
Signed-off-by: Derek Collison derek@nats.io
Resolves #1332
/cc @nats-io/core