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

Improve roles persistency #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dberzano
Copy link

Improve roles handling and persistency

  • Do not cache roles at start: persistency store might not be available yet and
    an empty response can be returned
  • Save roles back to the brain on change, except admin

Nick Woodward and others added 2 commits July 13, 2016 23:48
* Do not cache roles at start: persistency store might not be available yet and
  an empty response can be returned
* Save roles back to the brain on change, except `admin`
@dberzano
Copy link
Author

dberzano commented Jul 27, 2016

@ceci-woodward this should in principle provide a better solution to #36 and #37 than #38 and should also provide a fix to the issue described in my comment, if I am not overlooking anything...

@cecilia-sanare
Copy link

My changes in this PR shouldn't be merged due to it breaking backwards compatability.
I.E. storing roles separate from the users

@JSzaszvari
Copy link

What exactly does this break?

Applying this patch actually makes hubot-auth work. The fact that if you restart hubot all roles are lost/forgotten is a pretty big issue...

@faridnsh
Copy link

Can we merge this and release a new semver major version?

@JSzaszvari
Copy link

@alFReD-NSH I forked this a whole ago with the changes the changes above so it actually works. I'm not sure how this plugin is meant to be useful in the slightest without the ability to retain the roles information after a reboot, but here it is

https://www.npmjs.com/package/hubot-auth-persistent

@cecilia-sanare
Copy link

@JSzaszvari @dberzano @alFReD-NSH

I recall the breaking change was due to us moving the location redis would store the users roles.
Previously we would store them directly on the user object, now we store them in a roles obect.
This means that users who don't run into this issue will have their roles wiped out.

I'm guessing its a race condition where there are times that the
user isn't initialized and the roles can get wiped out.

Adding the role and verifying it exists

image

After a restart verifying the role still exists

image

After pulling in @dberzano change

image

Overall I like the change, but we should probably try and
pull the roles from the user object if they exist.

@cecilia-sanare
Copy link

Something like the following should work for migrating roles from the old format.

module.exports = (robot) ->
  robot.brain.on 'connected', () ->
    roles = robot.brain.get('roles')
    for id, user of robot.brain.users()
      if user.roles and user.roles.length
        console.info "converting #{id} to the new roles format..."
        roles[id] or= []
        userRoles = roles[id]
        userRoles.push role for role in user.roles when role not in userRoles
        delete user.roles
    roles = robot.brain.set('roles', roles)
  # ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants