#7894 Don't update merge request timestamps incorrectly

v1.3.1
closed
Heith Seewald
merge-requests (34) sf-2 (994)
General
Igor Bondarenko
2015-07-13
2015-06-09
Dave Brondsema
No

Merge request's "updated" timestamp changes when you view it and it calculates if a one-click merge is possible. That shouldn't happen. Make sure no other operations incorrectly update the timestamp either.

Discussion

1 2 > >> (Page 1 of 2)

  • Dave Brondsema

    Dave Brondsema - 2015-06-15
    • labels: merge-requests --> merge-requests, sf-2
     

  • Dave Brondsema

    Dave Brondsema - 2015-06-15
    • labels: merge-requests, sf-2 --> merge-requests, sf-2, sf-current
     

  • Heith Seewald - 2015-06-15
    • status: open --> in-progress
    • assigned_to: Heith Seewald
     

  • Heith Seewald - 2015-06-15
    • status: in-progress --> review
     

  • Heith Seewald - 2015-06-15

    Review at hs/7894

     

  • Igor Bondarenko - 2015-06-16
    • Reviewer: Igor Bondarenko
     

  • Igor Bondarenko - 2015-06-16
    • status: review --> in-progress
     

  • Igor Bondarenko - 2015-06-16

    This doesn't work for me. To test it properly you should disable caching inside def can_merge, like this:

    +#        if cached is not None:
+#            return cached

    This way task will be fired on every page hit.

    I think to properly fix this we should set skip_mod_date == True in the beginning of the repo_tasks.can_merge and set it back to False in the end. Maybe it can even go inside set_can_merge_cache(), so that no matter from which place cache is updated it will not trigger mod_date update.

    Also, we should consider adding context manager helper for this to the Allura, similar to one we use in SF internal code:

    @contextmanager
def skip_mod_date(model_cls):
    skip_mod_date = getattr(session(model_cls)._get(), 'skip_mod_date', False)
    session(model_cls)._get().skip_mod_date = True
    try:
        yield
    finally:
        session(model_cls)._get().skip_mod_date = skip_mod_date
     

  • Heith Seewald - 2015-06-16

    Ah, nice catch Igor, thanks.

     

  • Heith Seewald - 2015-06-29
    • status: in-progress --> review
     

  • Heith Seewald - 2015-06-29

    Review at:
    Allura hs/7894
    Forge-classic hs/7894

    I ended up moving skip_mod_date into Allura (allura.lib.utils).

     

  • Igor Bondarenko - 2015-07-01
    • status: review --> in-progress
     

  • Igor Bondarenko - 2015-07-01

    I'm getting errors like this from the task

    Traceback (most recent call last):
  File "/home/ibondarenko/ibondarenko-1154/forge/Allura/allura/model/monq_model.py", line 260, in __call__
    self.result = func(*self.args, **self.kwargs)
  File "/home/ibondarenko/ibondarenko-1154/forge/Allura/allura/tasks/repo_tasks.py", line 174, in can_merge
    mr.set_can_merge_cache(result)
  File "/home/ibondarenko/ibondarenko-1154/forge/Allura/allura/model/repository.py", line 854, in set_can_merge_cache
    with utils.skip_mod_date(self):
  File "/usr/lib64/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/home/ibondarenko/ibondarenko-1154/forge/Allura/allura/lib/utils.py", line 623, in skip_mod_date
    skip_mod_date = getattr(session(model_cls)._get(), 'skip_mod_date', False)
AttributeError: 'ODMSession' object has no attribute '_get'

     

    Also, do we really need with skip_mod_date inside both can_merge task and set_can_merge_cache. It seems to me that skipping it just in set_can_merge_cache should be sufficient. Or repo.can_merge also modifies merge request objects somehow?

     

  • Dave Brondsema

    Dave Brondsema - 2015-07-01

    Also I think skip_mod_date only works when the changes are flushed, not when you are setting the values. If that's correct, we might need to set it directly and leave it (not context manager), or think of some other approach.

     

    • Igor Bondarenko - 2015-07-01

      One option is flushing inside set_can_merge_cache, thus inside context manager

       

  • Heith Seewald - 2015-07-01
    • status: in-progress --> review
     

  • Heith Seewald - 2015-07-01

    force-pushed to: hs/7894

    I also added docstrings to skip_mod_date with some of the notes from this ticket.

     

  • Igor Bondarenko - 2015-07-02
    • status: review --> in-progress
     

  • Igor Bondarenko - 2015-07-02

    I've rebased on a new master and removed a couple of unused dependencies from previous commits. See ib/7894

    That works fine, but few tests are failing https://forge-allura.apache.org/p/allura/pastebin/5594fec86d19cd6b98fe5b42

     

    • Igor Bondarenko - 2015-07-02

      test_branches and test_tags are failing in master also :( There are one more test failure specific to this ticket, though

       

      • Dave Brondsema

        Dave Brondsema - 2015-07-02

        How do test_branches and test_tags fail for you? I saw them fail at Apache Jenkins also, but couldn't reproduce it locally. I thought maybe it was because I lowered the default cache threshold, and it was a timing thing. But I still couldn't reproduce it locally with a setting of 0 or -1.

         

        • Igor Bondarenko - 2015-07-02

          Individually they pass, they fail only when run using ./run-tests or:

          $ cd ForgeGit && nosetests forgegit.tests.model.test_repository
           

          • Dave Brondsema

            Dave Brondsema - 2015-07-02

            Thanks. I've fixed the failures on master.

             

  • Heith Seewald - 2015-07-02

    Force pushed to hs/7894

    tests.model.test_repo.TestMergeRequest are now passing.

     

  • Heith Seewald - 2015-07-02
    • status: in-progress --> review
     
1 2 > >> (Page 1 of 2)

Log in to post a comment.