Making the permissions of the local artifact cache configurable.

Review Request #3869 — Created May 11, 2016 and submitted

gmalmquist
pants
gmalmquist/local-artifact-cache-permissions
3406
pants-reviews
benjyw, patricklaw, stuhood, zundel

We ran into some internal problems due to the permissions of files
created for the local artifact cache being 0600 instead of 0644.

This is mostly a subset of https://rbcommons.com/s/twitter/r/3867/.

Added passing tests to test_contextutil.py.

CI went green here: https://travis-ci.org/pantsbuild/pants/builds/129562253

  1. 
      
  2. src/python/pants/util/contextutil.py (Diff revision 1)
     
     

    Unrelated to your change, but can you think of why we have try: finally: here? The whole point of contextmanager and yield is to not do that...

    1. Yeah. So actually if you do:

      @contextmanager
      def foobar():
        yield 'some value'
        print('We got to the end!')
      
      def use_foobar():
        with foobar():
          raise RuntimeError('Oh no!')
      
      use_foobar()
      

      The error will actually break the context manager, and 'We got to the end!' will never print. On the other hand, if you have:

      @contextmanager
      def foobar():
        try:
          yield 'some value'
        finally:
          print('We got to the end!')
      
      def use_foobar():
        with foobar():
          raise RuntimeError('Oh no!')
      
      use_foobar()
      

      The 'We got to the end!' will print before the exception blows up the process.

  3. 
      
  1. 
      
  2. Maybe the new parameter should be a named parameter so its optional? I know this isn't a public API so it isn't necessary.

  3. add pydoc for new parameter? maybe just a pointer to the other class

  4. Doesn't this constructor also need to be modified?

  5. Why not use a file permission that is easier for us to delete or more commonly used (0644? 0622?)

  6. 
      
  1. Ship It!
    1. FYI other reviewers - I've patched this in to our local pants version and things are running smoothly.

  2. 
      
  1. So Square will be setting this in the pants.ini, to something like 644? We aren't as strict, perhaps we should be.

    The permissions bit to the tempdirs seems like it could be useful in other places, but what is the use case for ever flipping the permissions of the artifact_cache as opposed to hardcoding?

    1. I didn't want to hardcode the local artifact cache to use mode 0644 because I had no reason to suspect that would be universally desirable, or even always the right answer for Square.

    2. Yeah, I can understand that. We don't use 644, for instance- we are using 755. But perhaps we could discuss a default on pants-devel.

      As it stands, this patch doesn't actually prevent the problem you mention above unless the repo has specificaly opted-in the solution. That suggests that either pants.ini should have a default or it should be hardcoded to something, imo.

      In general, I would prefer a use case when adding options, especially if there is no default :)

    3. The current default is 'safe' in that it can't accidentally leak information. You really only want other users to read the cache if you are using some kind of shared buildcache, which isn't always the case for a local filesystem.

    4. Okay, fair enough. I think that is a good defense of why it should be an option. I still would prefer a default but this is generally fine with me.

  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

In 0cb2d5599484604883684a0e503b376baa2c4469, Thanks Benjy, Eric, & Mateo

Loading...