search

chef / cookbook-omnifetch

Fetch Chef Cookbooks from Various Sources to a Local Cache
Apache License 2.0
6 stars 9 forks source link

Change artifact server to use a staging directory that is adjacent to th... #1

Closed ksubrama closed 9 years ago

ksubrama commented 9 years ago

...e cache directory.

danielsdeleo commented 9 years ago

Would be great to add test coverage for #install while we're here. Generally I find mocking the file system to be pretty painful, so I tend to avoid it, but do mock network calls. You should be able to set up the test so that CookbookOmnifetch.cache_path (and others) point to temporary directories you create in your test setup (so you can keep test data from interfering with real data, and also ensure it's cleaned up in an after step). I would tarball up a simple cookbook to use as fixture data, and then you can make expectations about the final content after #install runs. Let me know if you need help with any of this.

danielsdeleo commented 9 years ago

This is good :+1: Only thing is I'd prefer to get the TODO sorted out ahead of time rather than commit to the repo (where it will probably languish forever). If there's an actual problem there it'll probably get detected quickly enough that one of us would know where to look (and someone not familiar with it probably wouldn't know to look for a TODO in the code).

ksubrama commented 9 years ago

I agree - the untarred files should have sensible permissions inherited from the permissions of the staging directory. The original tarball will either have the permissions of the original temp file or will receive the correct permissions if it's copied. Unfortunately, I don't really know how to set permissions on the file in a cross-platform acceptable way. There is code here: https://github.com/chef/chef/blob/master/lib/chef/file_content_management/deploy/mv_windows.rb that tries to preserve the original permissions but that's windows specific and I don't think that's what we want here (we probably want to refresh and re-inherit the destination acls instead). On linux... should I be looking for the mode of the parent directory and setting it that way. Do we care handle linux extended ACLs?

I left the TODO there because in Windows, normally the temp directory is going to be under your User/AppData directory meaning you'll probably get sane permissions. On linux/mac, ruby by default created 0600 tempfiles with 0600 - so everything should work itself out but I wanted someone with the right security chops (not me) to double check that. In any case, it isn't a regressions from the current behavior. I'll drop the TODO for now and if an actual issue crops up, we can go dig into it then.