Retry on failed scm push. Also pull with rebase, to increase the odds of success. Don't pull before tagging.

Review Request #1083 — Created Sept. 26, 2014 and submitted

dturner-tw
pants
d01ee6b...
pants-reviews
ity, jsirois, stuhood

Retry on failed scm push. Also pull with rebase, to increase the odds of success. Don't pull before tagging.

In addition, separate commit from push; this means that SVN will no longer be supportable, but that's OK because SVN users could use git-svn (which is supportable if anyone cares).

This review is meant to apply on to of https://rbcommons.com/s/twitter/r/1082/ -- when we're happy with 1082, I'll rewrite this one as necessary.

Ran unit and integration tests for jar_publish. Also for git.

  • 0
  • 0
  • 4
  • 1
  • 5
Description From Last Updated
DT
ST
  1. 
      
  2. Since this was an --ff-only pull, this would have caused (correct) early failure in cases where people had invalid stuff committed to master. Do we still have some defense against that? Or are we just expecting to fail due to git-hooks if we need to?

    1. Doesn't check_clean_master check this?

  3. If this is going to be bounded, it should be configurable. But I'd prefer that it not be bounded (and thus not need to be configurable.)

    1. The reason I want to eventually fail is because we don't have good diagnostics about what the cause of the failure is, so it could be a config failure; then we'll never succeed. I'll make it configurable.

  4. I don't think you should print out the full stack trace except when you're giving up (if you decide to give up.) Otherwise it is non-obvious that the exception was expected and you're retrying.

  5. formatting

  6. src/python/pants/scm/git.py (Diff revision 2)
     
     
     

    The advantage to --ff-only was that it left the workspace in a clean state if it failed. What will happen if this rebase fails? We'd like it to succeed/fail atomically if possible.

    1. Good idea. I'll back the rebase out.

  7. formatting (and elsewhere)

  8. 
      
DT
DT
JS
  1. 
      
  2. src/python/pants/scm/scm.py (Diff revision 3)
     
     

    What does this mean for subversion? Granted - I should add svn support to help force the issue of the Scm abstraction, but that's its point - to allow svn, etc.
    This should really be done for Git's impl of commit. Maybe a defaulted kwarg that makes retires a 1st class idea for any Scm.commit - JarPublish could then pass the kwarg in a targeted way.

  3. 
      
DT
  1. 
      
  2. src/python/pants/scm/scm.py (Diff revision 3)
     
     

    Re subversion, see http://www.achewood.com/index.php?date=11222006

    But seriously, anyone who is stuck with a SVN server can still use git-svn on the client side.

    I don't think we should force everyone into the procrustean bed of SVN support just because of some hypothetical set of users that haven't complained yet.

    I'm a bit worried about moving retries into the SCM, because in the long run, it's not even the right thing to do in the publishing case. That is, we might have a persistent git conflict which does not represent a true pushdb conflict. To really retry for publishing, we want to check whether the changes obviate this set of changes, or require a version number bump, etc. This is basically a stopgap until we switch to content-based publishing (SHA dependencies).

    1. As a stop-gap, you also have set_scm [1] to locally install your very own HackyRetryingGit. The very idea of publishing so frequent it causes scm contention regularly is antithetical to pants; ie: your svn-stone-age comments apply to this hack as well.

      [1] https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/build_environment.py#L59

    2. It's not that publishing is so frequent -- it's that it takes a long time and there are likely to be unrelated intervening commits. If we're maintaining a pushdb in a SCM, we need to have retrying because there's always the possibility of intervening commits.

  3. 
      
JS
  1. 
      
  2. This is now just 'commit', 'commit_push' is misleading and make this logic appear broken, ie a succeeding 'commit_push' always leads to another self.scm.push()

    1. It's "commit a change to the pushdb", which I guess should be called "commit_pushdb"; changed it to that.

  3. How about just lifting up an scm_exception = None outside the loop. Presumably that works in all pythons and its clear code that would need no comment.

  4. Please just lift to the normal import section - there are bad examples of this style in the codebase and its unclear why we ever did this.

  5. This bit is broken: s/scm/(Scm|self.scm)/

    That noted, is this whole block just here to support the comment? How about kill the block and if you want to call out Scm.RemoteExceptions are not retried you can do that in the comment above the try.

  6. src/python/pants/scm/git.py (Diff revision 3)
     
     

    How about just pull --rebase --tags - IIUC that does everything you want here.

    1. I intentionally did it in two commands so that I could distinguish local and remote exceptions.

    2. OK - that makes sense.

  7. src/python/pants/scm/scm.py (Diff revision 3)
     
     

    period - and pull the """ down a line like the rest of the doc in this file.

  8. 
      
JS
  1. 
      
  2. tests/python/pants_test/scm/test_git.py (Diff revision 3)
     
     

    I find this pattern obscuring here and below. How about just:

    with self.assertRaises(Scm.LocalException):
      self.git.refresh(leave_clean=False)
    # The repo is dirty
    self.assertEquals(set(['README']), self.git.changed_files(include_untracked=True, from_commit='HEAD'))
    with environment_as(GIT_DIR=self.gitdir, GIT_WORK_TREE=self.worktree):
      subprocess.check_call(['git', 'reset', '--hard', 'HEAD'])
    

    This has the advantage of not masking in the case that self.git.changed_files(...) raised a LocalException.

  3. 
      
DT
JS
  1. Last small bits - this lgtm.

  2. tests/python/pants_test/scm/test_git.py (Diff revisions 3 - 4)
     
     

    -2 indent

  3. tests/python/pants_test/scm/test_git.py (Diff revisions 3 - 4)
     
     

    Ditto here, please un-obscure

  4. Indents are odd here down, +3 on the """ guts I think or just 'a single line' - I think it now fits.

  5. 
      
DT
ST
  1. Thanks David!

  2. 
      
PA
  1. 
      
  2. Use range rather than xrange. The latter doesn't exist in python 3, and it's only useful in python 2 when N > ~1e8

  3. src/python/pants/scm/git.py (Diff revision 5)
     
     

    Modifying this method significantly is a good time to add a detailed docstring.

  4. 
      
DT
PA
  1. Ship It!

    1. Would love to, but it depends on https://rbcommons.com/s/twitter/r/1082/ which still needs shipits.

  2. 
      
JS
  1. This will need a final diff rebased against master now that 1082 is in for the patch to apply cleanly.

  2. 
      
DT
JS
  1. In master @ https://github.com/pantsbuild/pants/commit/2c602db227c8894732f83feba2d321c5629b5448
    Please mark this review as submitted.

  2. 
      
DT
Review request changed

Status: Closed (submitted)

Loading...