-
Notifications
You must be signed in to change notification settings - Fork 147
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
feat(example): Make it possible to load user configuration from file #656
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #656 +/- ##
==========================================
+ Coverage 60.06% 62.09% +2.03%
==========================================
Files 80 81 +1
Lines 6998 6218 -780
==========================================
- Hits 4203 3861 -342
+ Misses 2498 2047 -451
- Partials 297 310 +13 ☔ View full report in Codecov by Sentry. |
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.
Left 1 comment.
Also please change the PR description to match semver: feat(example): <description>
example/server/users.json
Outdated
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.
This file doesn't need to be included, right? The defaults are already hard-coded in the program.
If you do think it is useful to include the file as an example, then use the embed
package and remove the hard-coded data from the storage/user.go
file.
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.
That is the testing data, I decided to include it as a sample for end users. If that is against some conventions, it's totally ok to remove it.
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.
It's duplicate data, the convention is called DRY. So please remove it or embed
it.
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.
I'm very sorry for closing this branch. I wanted to pull latest version before submitting a proper pull request, but removed my own code.
I'll try to submit a new one soon.
This change allows to load users from the json file.
That should help to create better test scenarios when using example server as fake Zitadel instance for testing. For example, having several users with different permissions or presence in the internal base (known user, unknown user, blocked user, etc).
We don't need dynamic server for testing, but I could update it to make env variables unified if needed.