-
Notifications
You must be signed in to change notification settings - Fork 987
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
Hostgroup add ancestors fix #576
Conversation
[test] |
looks good, do you mind adding a test? |
@elobato that test doesn't fail for me, as the hostgroups are getting added by the admin user (not :one), so the user doesn't get added in line 61 since it's the admin. How about this? https://github.com/domcleal/foreman/compare/576-hostgroup-ancestors |
@domcleal That looks great, I'm not sure if it's possible to merge your branch into this pull request and keep the attributions for your work, is it? If it's not feel free to close this one and open a new one with that, that test helper looks useful to me. |
@elobato don't worry about attribution if you're happy with it, just squash it into your commit and force push this branch. |
Test duplicate users in inherited hostgroup Test helper to setup users with operations (Dominic)
[test] |
[test] - it'll only let people in the org do it |
@ohadlevy ah, but his membership's hidden. If it's public then the bot can detect it (there was a change to the GH API recently that means it can now only see public people). |
Windows does not support non-blocking read on pipes - which causes read_noblock to throw Errno::EWOULDBLOCK, which is not handled in resque block.
Basically this pull request fixes the problem where:
an User is creating a new hostgroup
this user owns any of its ancestors
the Hostgroup creation will work, but shows an error "User is already in this Hostgroup", because indeed the User already owns that Hostgroup (that happens in line 61) and you are trying to add the User to this Hostgroup twice.