Bug #5989

[API] API server repositories permissions list doesn't list repositories without keys

Added by Ward Vandewege over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
API
Target version:
Start date:
07/02/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.50 h)
Story points:
0.5

Description

bug 1: it does not create the 'auto' subdirectory if it does not exist yet:

Error: No such file or directory @ rb_sysopen - /usr/local/arvados/gitolite-tmp/gitolite-admin/conf/auto/XXXXX-s0uqq-asdfasdfadsfaafds.conf20150510-28428-1xbndn3.tmp
/usr/local/rvm/rubies/ruby-2.1.5/lib/ruby/2.1.0/tempfile.rb:146:in `initialize'
/usr/local/rvm/rubies/ruby-2.1.5/lib/ruby/2.1.0/tempfile.rb:146:in `open'
/usr/local/rvm/rubies/ruby-2.1.5/lib/ruby/2.1.0/tempfile.rb:146:in `block in initialize'
/usr/local/rvm/rubies/ruby-2.1.5/lib/ruby/2.1.0/tmpdir.rb:142:in `create'
/usr/local/rvm/rubies/ruby-2.1.5/lib/ruby/2.1.0/tempfile.rb:136:in `initialize'
/usr/local/arvados/update-gitolite.rb:62:in `new'
/usr/local/arvados/update-gitolite.rb:62:in `replace_file'
/usr/local/arvados/update-gitolite.rb:95:in `ensure_in_git'
/usr/local/arvados/update-gitolite.rb:103:in `ensure_in_git'
/usr/local/arvados/update-gitolite.rb:186:in `ensure_config'
/usr/local/arvados/update-gitolite.rb:242:in `block in <main>'
/usr/local/arvados/update-gitolite.rb:240:in `each'
/usr/local/arvados/update-gitolite.rb:240:in `<main>'

bug 2: it does not create any defined repositories on disk unless there is at least one ssh key associated with a user in the system. This is problematic because the 'arvados' repo is not created on disk.


Subtasks

Task #6291: Review 5989-bug_1ResolvedBrett Smith

Task #6479: Review 5989-api-all-repos-permissions-wipResolvedBrett Smith

Associated revisions

Revision 34ec4990 (diff)
Added by Nico César over 6 years ago

5989: create directory structure for temp files

refs #5989 bug_1

Revision 76e74a7f
Added by Nico César over 6 years ago

Merge branch '5989-bug_1' merge with puppet repo "copy and paste" to sync up duplicated code.

refs #5989

Revision 9df4975a (diff)
Added by Nico César over 6 years ago

5989: create directory structure for temp files

refs #5989 bug_1

Revision 5608a875
Added by Brett Smith over 6 years ago

Merge branch '5989-api-all-repos-permissions-wip'

Closes #5989, #6479.

History

#1 Updated by Ward Vandewege over 6 years ago

  • Description updated (diff)

#2 Updated by Brett Smith over 6 years ago

  • Subject changed from 2 small update-gitolite.rb script bugs to [API] Two small update-gitolite.rb bugs around new installs
  • Category set to API
  • Target version changed from Bug Triage to Arvados Future Sprints

#3 Updated by Brett Smith over 6 years ago

  • Target version changed from Arvados Future Sprints to 2015-07-08 sprint

#4 Updated by Brett Smith over 6 years ago

  • Story points set to 0.5

#5 Updated by Brett Smith over 6 years ago

  • Assigned To set to Nico César

#6 Updated by Nico César over 6 years ago

  • Status changed from New to In Progress

brach 5989-bug_1 should thave the fix. How do we test this?

#7 Updated by Nico César over 6 years ago

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.

#8 Updated by Brett Smith over 6 years ago

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.

#9 Updated by Brett Smith over 6 years ago

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

#10 Updated by Brett Smith over 6 years ago

  • Subject changed from [API] Two small update-gitolite.rb bugs around new installs to [API] API server repositories permissions list doesn't list repositories without keys

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.

#11 Updated by Brett Smith over 6 years ago

  • Assigned To changed from Nico César to Brett Smith

#12 Updated by Brett Smith over 6 years ago

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.

#13 Updated by Nico César over 6 years ago

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.

#14 Updated by Brett Smith over 6 years ago

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.

#15 Updated by Brett Smith over 6 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100

Applied in changeset arvados|commit:5608a875c36101c791e35c474abf5e3900aad071.

Also available in: Atom PDF