Arvados

Noticed virtual_machines_controller.rb now has

          username = perm.properties.andand['username']
...
              groups: (perm.properties["groups"].to_a rescue []),

If it's safe to assume properties is always a hash, never nil (which is assured by serialize :properties, Hash in source:services/api/app/models/link.rb and the serialize validation in source:services/api/app/model/arvados_mode.rb), then this might be a good time to get rid of the old .andand and make it more clear that the only purpose of the new "rescue" is to silently ignore cases where properties["groups"] is a Hash, String, Fixnum, etc. (Assuming that was your intent...)

Tom Clegg wrote:

If it's safe to assume properties is always a hash, never nil (which is assured by serialize :properties, Hash in source:services/api/app/models/link.rb and the serialize validation in source:services/api/app/model/arvados_mode.rb), then this might be a good time to get rid of the old .andand and make it more clear that the only purpose of the new "rescue" is to silently ignore cases where properties["groups"] is a Hash, String, Fixnum, etc. (Assuming that was your intent...)

You understood correctly. Implemented your suggestion in a9dcceaa. Thanks.

checked a9dcceaa2d3dcc3ff8984ac013022fda5a1a31e9

"run-tests.sh --only services/api" tested it successfully:

Arvados::V1::VirtualMachinesControllerTest#test_groups_is_an_empty_list_by_default = 0.04 s = .
Arvados::V1::VirtualMachinesControllerTest#test_groups_propagated_from_permission = 0.04 s = .
Arvados::V1::VirtualMachinesControllerTest#test_logins_without_usernames_not_listed = 0.05 s = .
Arvados::V1::VirtualMachinesControllerTest#test_username_propagated_from_permission = 0.04 s = .

I also went thru the code and "LGTM". merge it!

I'm reviewing 0a367d7f69abc3897a1ab557e8c14b71f2ec456e from puppet repo

I see that

modules/arvados-shell-server/templates/usr-local-arvados-update-shell-accounts.rb.erb

has hardcoded 1000 as first user in line 60. that's a variable. should be taken from:

grep ^UID_MIN /etc/login.defs

and This is having the desired effect:

1. No system users
   - logins = logins.reject { |l|
   - id = `id u #{l[:username]} 2>/dev/null`.to_i
   $?.to_i == 0 and id < 1000
   - }
   + logins.reject! { |name| (uids[name] || 65535) < 1000 }

Nico Cesar wrote:

I'm reviewing 0a367d7f69abc3897a1ab557e8c14b71f2ec456e from puppet repo

I've updated the branch to address both your comments. Thanks especially for catching that bug around rejecting system logins. Now at 5140a12. Thanks.

revision 5140a12c784d1c2372d6c9d5b533794e3515e38a LGTM tested on shell.4xphq and DIDN'T wipped out my .ssh/authorized key. I'm happy now.