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

Remove subject_transform_dest #4557

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

jnmoyne
Copy link
Contributor

@jnmoyne jnmoyne commented Sep 19, 2023

Removes the single subject transform destination field any subject transformation in StreamSources must now be done using the SubjectTransforms array instead.

@jnmoyne jnmoyne requested a review from a team as a code owner September 19, 2023 03:19
server/stream.go Outdated Show resolved Hide resolved
@wallyqs wallyqs changed the title jnm/remove_single_transform_from_stream_sources Remove SubjectTransformDest, should use SubjectTransforms array instead Sep 19, 2023
wallyqs

This comment was marked as outdated.

wallyqs

This comment was marked as outdated.

jnmoyne added a commit to nats-io/nats.go that referenced this pull request Sep 19, 2023
jnmoyne added a commit to nats-io/jsm.go that referenced this pull request Sep 19, 2023
jnmoyne added a commit to nats-io/jsm.go that referenced this pull request Sep 19, 2023
jnmoyne added a commit to nats-io/jsm.go that referenced this pull request Sep 19, 2023
jnmoyne added a commit to nats-io/jsm.go that referenced this pull request Sep 19, 2023
piotrpio pushed a commit to nats-io/nats.go that referenced this pull request Sep 19, 2023
@neilalexander neilalexander marked this pull request as draft September 19, 2023 10:44
@derekcollison
Copy link
Member

If someone has a stream that had this configured and that was saved into the stream config on disk, what happens to them?

piotrpio pushed a commit to nats-io/nats.go that referenced this pull request Sep 19, 2023
@neilalexander neilalexander added the post-freeze We'll come back to this after the freeze period label Sep 19, 2023
neilalexander added a commit that referenced this pull request Sep 19, 2023
This is a safer (less lines of code touched) alternative to #4557 for
now, which simply ignores the `subject_transform_dest` field in the API
and the stream assignments. We'll still look to merge the other PR to
clean up but will do so post-release when we have more time to test it.

Signed-off-by: Neil Twigg <neil@nats.io>
piotrpio pushed a commit to nats-io/nats.go that referenced this pull request Sep 20, 2023
Co-authored-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Co-authored-by: Neil Twigg <neil@nats.io>

Signed-off-by: Neil Twigg <neil@nats.io>
@neilalexander neilalexander force-pushed the jnm/remove_single_transform_from_stream_sources branch from f336a31 to 9fc2603 Compare September 20, 2023 14:31
@neilalexander
Copy link
Member

Rebased on top of main. Also removed the field from the struct altogether, including the error if it is used, as 2.10.0 ignores the field in the stream config JSON anyway.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM - if tests pass.

@neilalexander neilalexander changed the title Remove SubjectTransformDest, should use SubjectTransforms array instead Remove subject_transform_dest Sep 20, 2023
@neilalexander neilalexander marked this pull request as ready for review September 20, 2023 14:45
@neilalexander neilalexander removed the post-freeze We'll come back to this after the freeze period label Sep 20, 2023
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM now that the TestJetStreamFilteredSubjectUsesNewConsumerCreateSubject test exists and passes.

@neilalexander neilalexander merged commit 0623e4b into main Sep 20, 2023
2 checks passed
@neilalexander neilalexander deleted the jnm/remove_single_transform_from_stream_sources branch September 20, 2023 14:57
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

4 participants