Arvados

About bug 2 on line 233 I see:

  permissions[:repositories].each do |repo_record|
    repo = Repository.new(repo_record, user_ssh_keys)
    repo.ensure_config(gitolite_admin)
  end

That's Brett's code from 39b48294

why aren't we iterating over permissions instead of repositories?

a quick dirty fix will just be:

  if not Repository.arvados_exists?
    Repository.new("arvados")
    repo.ensure_config(gitolite_admin)
  end

but I want to make sure I'm doing the right thing and not an ugly patch and there are real reasons why we're doing that iteration.

Nico Cesar wrote:

About bug 2 on line 233 I see:

[...]

That's Brett's code from 39b48294

why aren't we iterating over permissions instead of repositories?

We are iterating over permissions instead of repositories, and that's exactly the problem: the API method that we call to get the source information does not return any information about repositories that don't have an associated SSH key. We should consider whether it makes more sense to change the API server to return that data—if we do, the script basically fixes itself. Alternatively, we could teach the script to query the full list of repositories, make sure they're all defined in gitolite, then propagate keys based on this API response—roughly along the lines of your second suggestion.

The code in 5989-bug_1 is good. See e-mail about deployment issues. Thanks.

Brett Smith wrote:

Brett Smith wrote:

We are iterating over permissions instead of repositories, and that's exactly the problem: the API method that we call to get the source information does not return any information about repositories that don't have an associated SSH key. We should consider whether it makes more sense to change the API server to return that data—if we do, the script basically fixes itself.

Tom says yes. I will work on that change.

Done in branch 5989-api-all-repos-permissions-wip, up for review.

reviewed 0e239ef805b8c6eda936073197cf96d0329db048

This the API change looks good to me.

BTW: I see

-    User.includes(:authorized_keys).all.each do |u|
+    User.includes(:authorized_keys).find_each do |u|

nice. .find_each should be everywhere.

Nico Cesar wrote:

nice. .find_each should be everywhere.

Thanks for the review. find_each actually shouldn't be used everywhere, because it ignores limits and I think the ordering can get funky too. See 602706ef5510b3f07fc5fa988019952d2133320c for an example where using find_each was a bug that got fixed. But for cases like this where we know we're going to iterate everything, we don't need the whole list, and we don't care about order, yeah, it's much better.