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

[#SHIRO-875] Fix creating subjects from a SubjectFactory that disables session-creation #1514

Merged
merged 1 commit into from
Jun 3, 2024
Merged

[#SHIRO-875] Fix creating subjects from a SubjectFactory that disables session-creation #1514

merged 1 commit into from
Jun 3, 2024

Conversation

boris-petrov
Copy link
Contributor

I'm not sure if I had to remove the PR template after I've read it and made sure I followed everything in it. Perhaps you might want to note that in it.

This continues the work done in #1407. I didn't think of that case in it so this PR fixes it. I didn't open a new issue for it, sorry, I guess we could reuse the old one as it's a similar case.

cc @lprimak

Copy link
Member

@bdemers bdemers left a comment

Choose a reason for hiding this comment

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

This change looks good, but the test doesn't look like it's asserting that no session is being created? Can the test be improved (or commented if i'm reading it wrong)

@boris-petrov
Copy link
Contributor Author

@bdemers thanks for the feedback! The test is pretty much the same as the one above it - it doesn't do anything with the session on its own but the buildSubject call fails without the fix (as it calls save which tries creating a session). I could add a comment, sure.

Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

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

@boris-petrov thanks for your contribution!

@lprimak lprimak merged commit f49e89f into apache:main Jun 3, 2024
21 of 22 checks passed
@boris-petrov boris-petrov deleted the fix-creating-subjects-with-special-subject-factory branch June 3, 2024 21:33
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