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

fix: make NewSession behave same way like UpdateSession method #594

Merged
merged 12 commits into from
Dec 9, 2024

Conversation

xmlking
Copy link
Contributor

@xmlking xmlking commented Dec 6, 2024

Before submitting this PR:

Noticed session creation behave is different when UpdateSession method is called vs NewSession method is called.
keeping the behavior same for both methods, producing similar allowedRoles in session.

Checklist

  • No breaking changes
  • Tests pass
  • New features have new tests
  • Documentation is updated

Breaking changes

Avoid breaking changes and regressions. If you feel it is unavoidable, make it explicit in your PR comment so we can review it and see how to handle it.

Tests

  • please make sure your changes pass the current tests (Use the make test or the make watch command).
  • if you are introducing a new feature, please write as much tests as possible.

Documentation

Please make sure the documentation is updated accordingly, in particular:

@xmlking xmlking changed the title fix: make NewSession behave same way like UpdateSession method fix: make UpdateSession behave same way like NewSession method Dec 7, 2024
@xmlking xmlking changed the title fix: make UpdateSession behave same way like NewSession method fix: make NewSession behave same way like UpdateSession method Dec 7, 2024
@xmlking
Copy link
Contributor Author

xmlking commented Dec 9, 2024

the patch is verified with this testing project
https://github.com/xmlking/nhost-multi-tenancy-experiment

@dbarrosop
Copy link
Member

dbarrosop commented Dec 9, 2024

LGTM we need to make the linter happy though, I'd suggest just disabling the funlen check like in the UpdateSession method so just add //nolint:funlen after NewSession (

Also, rebase so you get the CI fix from here: #596

@dbarrosop dbarrosop merged commit 457de9a into nhost:main Dec 9, 2024
6 checks passed
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.

2 participants