8000 fix: wrap the log printing in buildRoleLinks() by 00LT00 · Pull Request #615 · casbin/casbin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: wrap the log printing in buildRoleLinks() #615

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 4, 2020
Merged

fix: wrap the log printing in buildRoleLinks() #615

merged 2 commits into from
Oct 4, 2020

Conversation

00LT00
Copy link
Member
@00LT00 00LT00 commented Oct 2, 2020 •
edited by hsluoyz
Loading

Signed-off-by: 00LT00 lightning@zerokirin.online

Fix: #614

@00LT00
fix: wrap the log printing in buildRoleLinks()
9fc86cf 
Signed-off-by: 00LT00 <lightning@zerokirin.online>
@hsluoyz
Copy link
Member
hsluoyz commented Oct 2, 2020

@holayao hi, @00LT00 already made a PR. Please review.

nodece
nodece reviewed Oct 3, 2020
View reviewed changes
nodece
nodece reviewed Oct 3, 2020
View reviewed changes
@00LT00
fix: move IsEnable() to PrintPolicy()
a5c8 8000 efc 
Signed-off-by: 00LT00 <lightning@zerokirin.online>
@nodece nodece merged commit d0d65d8 into casbin:master Oct 4, 2020
@github-actions
Copy link
github-actions bot commented Oct 4, 2020

🎉 This PR is included in version 2.13.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@holayao
Copy link
holayao commented Oct 9, 2020 •
edited
Loading

@nodece I was on vacation the other day, and I'm sorry I didn't review the code. I dont think this PR fixs the problem i am facing . The line which takes up a large heap is in this function as here. And it seems like this fuction does nothing but print log. So is it possible to check log flag before execute this function? Thanks

@nodece
Copy link
Member
nodece commented Oct 9, 2020

@holayao We have added checks.

casbin/rbac/default-role-manager/role_manager.go

Line 231 in d0d65d8

if log.GetLogger().IsEnabled() {

@holayao
Copy link
holayao commented Oct 9, 2020

@nodece Sorry, i didn't notice that .Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nodece nodece nodece left review comments

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Question]Is it possible to ignore func (rm *RoleManager) PrintRoles() when i disable log
4 participants
@00LT00 @hsluoyz @holayao @nodece
0