Merge lp://qastaging/~dpb/tarmac/set-milestone-2 into lp://qastaging/tarmac

Proposed by David Britton
Status: Merged
Approved by: dobey
Approved revision: 441
Merged at revision: 432
Proposed branch: lp://qastaging/~dpb/tarmac/set-milestone-2
Merge into: lp://qastaging/tarmac
Diff against target: 477 lines (+381/-3)
5 files modified
docs/introduction.txt (+16/-0)
tarmac/plugins/__init__.py (+18/-0)
tarmac/plugins/bugresolver.py (+106/-0)
tarmac/plugins/tests/test_bugresolver.py (+196/-3)
tarmac/plugins/tests/test_tarmac_plugin.py (+45/-0)
To merge this branch: bzr merge lp://qastaging/~dpb/tarmac/set-milestone-2
Reviewer Review Type Date Requested Status
dobey Approve
Chad Smith Approve
Данило Шеган (community) Approve
Review via email: mp+210888@code.qastaging.launchpad.net

Commit message

Add an option to bugresolver to set the milestone if not already set.

Description of the change

When resolving a bug, set the milestone if not already set.

Milestone is selected using the following algorithm:

1) nearest milestone with target date in the future
2) if no milestones are in the future, sort remaining open milestones lexically by name and pick the first
3) If all milestones have dates and we are past all dates, pick the newest of those.
4) override all that logic with a config setting of "default_milestone" which is the specific milestone name to use (which will even select a non-active milestone).

This behavior only engages if the config setting 'set_milestone = True" is present in your config file.

You can test with: trial -j4 tarmac (or ./run-tests)

Read "Hacking" for a small list of deps needed for testing.

To post a comment you must log in.
Revision history for this message
dobey (dobey) wrote :

Can you please file bugs for new features, and link them? You'll also need to set the commit message.

review: Needs Fixing
Revision history for this message
David Britton (dpb) wrote :

On Thu, Mar 13, 2014 at 07:42:35PM -0000, Rodney Dawes wrote:
> Review: Needs Fixing
>
> Can you please file bugs for new features, and link them? You'll also need to set the commit message.

Yes, but we want to do an internal review first, so I hadn't done
everything yet. Please feel free to ignore until we get there.

--
David Britton <email address hidden>

Revision history for this message
Данило Шеган (danilo) wrote :
Download full text (17.0 KiB)

Hi David,

Thanks for taking on this. The branch looks pretty good, though, with
tarmac not being code-base I am familiar with, I'll try to infer some of
the policies from the surrounding code and make a few suggestions along
the way (mostly for code structure and tests).

The review is going to be long, but mostly mechanical/cosmetical stuff.

I simply can't restrain myself suggesting a more optimal approach
sometimes, even though it probably makes no difference for small numbers
of milestones most everybody is going to have. That means that you need
not follow up on any of them :)

  review needs-fixing

Cheers,
Danilo

> === modified file 'docs/introduction.txt'
> @@ -263,6 +263,22 @@
...
> + # the one that sorts least amoung them the target. If this
algorithm

s/amoung/among/

> === modified file 'tarmac/plugins/__init__.py'
> + def get_config(self, attribute, default, *args):
...
> + if hasattr(obj, "config"):
> + if hasattr(obj.config, attribute):

You can simplify this as

            if hasattr(obj, "config") and hasattr(obj.config,
attribute):

to save one indentation level (Python will not evaluate right side of
"and" when the left one evaluates to False). Not a big deal here, but
can be handy when your ifs start getting deeply nested.

I am not sure this method really belongs as-is in the TarmacPlugin
either: it seems to be very generic (eg. no mention of "self" in the
body, and it's not used anywhere inside TarmacPlugin methods). At the
very least, it should be a function.

I don't see unit tests for this method either (and they should be pretty
simple to write).

> === modified file 'tarmac/plugins/bugresolver.py'
> + def _get_and_parse_config(self, *args):
> + """
> + Retrieve and parse configuration settings for this plugin.
> + Return as a dict structure.
> + """
> + set_milestone = self.get_config("set_milestone", "False",
*args)
> + if set_milestone.lower() == "true" or set_milestone == "1":
> + set_milestone = True
> + else:
> + set_milestone = False
> +
> + default = self.get_config("default_milestone", None, *args)
> + if default is not None and not len(default):
> + default = None

Why check for len() here? I assume you wanted default == "", so I
suggest you be explicit. It'd also make the check simpler since it'd be
just

  if default == "":
      default = None

> + def _set_milestone_on_task(self, project, task):
...

I know you are testing this with test_run_with_set_milestone() but it'd
be good to have a unit test or two for this as well. I can see a few
cases here:

 - config option not set, no-op on task milestone
 - milestone already set, no-op on task milestone, warning logged
 - config option set, milestone is being set, action logged

> + def _find_milestones(self, project):
> + """
> + Return list of milestones in a project. Filter list by
active status.
> + If the config `default_milestone` is set filter by that
instead.
> + """
> + default = self.config["default_milestone"]
> + milestones = []
> + for milestone in pro...

review: Needs Fixing
Revision history for this message
David Britton (dpb) wrote :
Download full text (15.1 KiB)

On Wed, Mar 19, 2014 at 05:09:17PM -0000, Данило Шеган wrote:
> Review: Needs Fixing

Wow, thanks for the Review Danilo -- very helpful. I implemented most
of your suggestions, but left a couple on the table. I'll note below...

Please re-review when you get a chance.

> s/amoung/among/

Fixed.

>
> > === modified file 'tarmac/plugins/__init__.py'
> > + def get_config(self, attribute, default, *args):
> ...
> > + if hasattr(obj, "config"):
> > + if hasattr(obj.config, attribute):
>
> You can simplify this as
>
> if hasattr(obj, "config") and hasattr(obj.config, attribute):

Fixed.

>
> I am not sure this method really belongs as-is in the TarmacPlugin
> either: it seems to be very generic (eg. no mention of "self" in the
> body, and it's not used anywhere inside TarmacPlugin methods). At the
> very least, it should be a function.

Thanks -- I changed it to a @staticmethod. I'm willing to move it, but
I can't really find a good place. I saw this type of logic repeated in
many plugins, so I figured the plugins base class would be a good place
to start. I guess it can always be moved in the future. Let me know...

>
> I don't see unit tests for this method either (and they should be pretty
> simple to write).

Fixed.

>
> > === modified file 'tarmac/plugins/bugresolver.py'
> > + def _get_and_parse_config(self, *args):
> > + """
> > + Retrieve and parse configuration settings for this plugin.
> > + Return as a dict structure.
> > + """
> > + set_milestone = self.get_config("set_milestone", "False", *args)
> > + if set_milestone.lower() == "true" or set_milestone == "1":
> > + set_milestone = True
> > + else:
> > + set_milestone = False
> > +
> > + default = self.get_config("default_milestone", None, *args)
> > + if default is not None and not len(default):
> > + default = None
>
> Why check for len() here? I assume you wanted default == "", so I
> suggest you be explicit. It'd also make the check simpler since it'd be
> just
>
> if default == "":
> default = None

Fixed.

>
> > + def _set_milestone_on_task(self, project, task):
> ...
>
> I know you are testing this with test_run_with_set_milestone() but it'd
> be good to have a unit test or two for this as well. I can see a few
> cases here:
>
> - config option not set, no-op on task milestone
> - milestone already set, no-op on task milestone, warning logged
> - config option set, milestone is being set, action logged

Good catch. Fixed.

>
> > + def _find_milestones(self, project):
> > + """
> > + Return list of milestones in a project. Filter list by
> active status.
> > + If the config `default_milestone` is set filter by that
> instead.
> > + """
> > + default = self.config["default_milestone"]
> > + milestones = []
> > + for milestone in project.all_milestones:
>
> You can optimize by fetching only active_milestones.

Fixed. I didn't know about active_milestones. Indeed, that simplified
much of the code.

>
> > + if default is not None:
> > + ...

Revision history for this message
Данило Шеган (danilo) wrote :

Hi David,

Thanks for bearing with me. Most everything looks good, but you named your new test file

  test_tarmac_pluign.py

The rest is really good now, thanks.

  review approve

> OK, I took your suggestion. Though I still like the previous one as I
> found it easier to follow. I was curious, so I timed some stuff:
>
> 1000 milestones, 10,000 iterstions:
>
> old: 46.1424999237
> new: 44.1123301983
>
> 1,000,000 milestones, 10 iterations:
>
> old: 57.2593920231
> new: 49.6416790485
>
> As expected, n log n nature of the sort is slowing things down when large
> amounts of records come into play. And... I think this is quickly
> entering a theoretical discussion. :)

Heh, indeed. Thanks for testing it out: the difference might rather amount to
Python memory management (you are still dealing with duplicated structures in
the original variant) since the loop is much more complicated (and thus
slower) in "my" proposal (and sorted() is heavily optimized in Python).

So, in the end, it was probably not worth it even for large sets of
milestones, but it was fun nevertheless :)

review: Approve
Revision history for this message
Chad Smith (chad.smith) wrote :

[1] in test_bugresolver setUp:
  Not that it'll take a long time to run setUp, but we should move self.now to the top and use self.now when creating all the self.milestone_* attributes so they are all based on the same now.
 247 + self.now = datetime.utcnow()

[2]test_run_with_set_milestone docstr is unhelpful with the expectations of what "correctly" is. Maybe a bit more to explain why 1 milestone is left None, one added and the 3rd left unchanged.
 + """Test plug-in with the set_milestone setting runs correctly."""

[3] test_run_with_set_milestone can use self.assertIsNone()
277 + self.assertEqual(self.bugs['0'].bug_tasks[1].milestone, None)

[4] In test__find_target_milestone_between can we have an assert to prove that the date is between milestone_past and milestone_future? Something like:

now = self.milestone_past.date_targeted + timedelta(weeks=1)
self.assertTrue(now , self.milestone_future)
milestone = self.plugin._find_target_milestone(
     self.projects[1], now)

[5] variable naming for in _get_and_parse_config() clarity default -> default_milestone
96 + default = self.get_config("default_milestone", None, *args)
97 + if default == "":
98 + default = None

[6] docstring fix in _find_target_milestone
            3) the last milestone in the list (covers len()==1 case).
            "the last" -> "the most recent"

Revision history for this message
Chad Smith (chad.smith) wrote :

Ran through a few live trials of this with a development project and was able to validate lexical milestone setting, leaving existing milestones unchanged and assigning active milestones at the closest specific dates. Nice job +1!

review: Approve
Revision history for this message
David Britton (dpb) wrote :

OK, I've deployed to our jenkins environment. We will get some use of it then submit upstream for inclusion.

Revision history for this message
David Britton (dpb) wrote :

We've been running this now for 2 months, it's working well and is an optional feature. Asking for inclusion in upstream.

Revision history for this message
dobey (dobey) wrote :

> We've been running this now for 2 months, it's working well and is an optional
> feature. Asking for inclusion in upstream.

OK. It'll probably be a couple weeks more before I can get to reviewing it, due to sprint and vacation. I'll try to review it as soon as I can though.

Revision history for this message
David Britton (dpb) wrote :

Thanks!

On Fri, May 23, 2014 at 2:26 PM, Rodney Dawes <email address hidden>wrote:

> > We've been running this now for 2 months, it's working well and is an
> optional
> > feature. Asking for inclusion in upstream.
>
> OK. It'll probably be a couple weeks more before I can get to reviewing
> it, due to sprint and vacation. I'll try to review it as soon as I can
> though.
> --
>
> https://code.launchpad.net/~davidpbritton/tarmac/set-milestone-2/+merge/210888
> You are the owner of lp:~davidpbritton/tarmac/set-milestone-2.
>

--
David Britton <email address hidden>

Revision history for this message
dobey (dobey) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches