Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad
+def runGerminate(self, override_file, series_name, arch, flavours,
+ structures):
Rightly or wrongly, the “run” in that name led me to expect that this method
would fire up a separate process. Maybe a very short docstring to dispel that
notion?
+germinator = Germinator(arch)
+
+# Read archive metadata.
+archive = TagFile(
+series_name, self.components, arch,
+'file://%s' % self.config.archiveroot, cleanup=True)
+germinator.parse_archive(archive)
+
+for flavour in flavours:
To me, your branch is of very high quality but this is its Achilles' heel. The
following loop body is massive! I can't see it all on one page, so it becomes
hard even to see just how long it is. If I judged the indentations right, it
makes up the entire rest of the method.
Please extract it. It's well nigh impossible to see (now or in the future,
after some maintenance) whether any variables are carried across loop
iterations. That sort of thing can easily trip you up, especially when it
happens by accident.
Once you've extracted the loop body, given its size, it's probably still worth
extracting chunks from that.
According to some, the pattern of “comment, block of code, blank line, comment,
block of code, blank line” is a strong indication that you need to split things
up. It shows that you have already done so in your own mind, but leaves each
individual reader to figure out what the dependencies between consecutive
blocks are.
Also, of course, small methods with simple purposes are easier to specify in
detail and cheaper to test thoroughly.
+self.logger.info('Germinating for %s/%s/%s',
+ flavour, series_name, arch)
We follow a different rule for line-breaking function calls than we do for
function definitions.
For calls, line-break right after the opening parenthesis and then indent the
arguments list by 4 spaces:
self.logger.info(
Germinating for %s/%s/%s., flavour, series_name, arch)
+# Add this to the germinate log as well so that that can be
+# debugged more easily. Log a separator line first.
+self.germinate_logger.info('', extra={'progress': True})
+self.germinate_logger.info('Germinating for %s/%s/%s',
+ flavour, series_name, arch,
+ extra={'progress': True})
What does the “extra” parameter do?
+# Expand dependencies.
+structure = structures[flavour]
+germinator.plant_seeds(structure)
+germinator.grow(structure)
+germinator.add_extras(structure)
+# Write output files.
+
+# The structure file makes it possible to figure out how the
+# other output files relate to each other.
+structure.write(self.outputPath(
+flavour, series_name, arch, 'structure'))
+
+# all and all.sources list the full set of binary and source
+# packages respectively for a given flavour/suite/architecture
+# combination.
+all_path = self.outputPath(flavour, series_name, arch, 'all')
+all_sources_path = self.outputPath(
+flavour, series_name, arch, 'all.sources')
+germinator.write_all_list(structure, all_path)
+germinator.write_all_source_list(structure, all_sources_path)
+
+# Write the dependency-expanded output for each seed. Several
+# of these are used by archive administration tools, and others
+# are useful for debugging, so it's best to just write them all.
+for seedname in structure.names:
+germinator.write_full_list(
+structure,
+self.outputPath(flavour, series_name, arch, seedname),
+seedname)
+
+def writeOverrides(seedname, key, value):
+packages = germinator.get_full(structure, seedname)
+for package in sorted(packages):
+print override_file, '%s/%s %s %s' % (
+package, arch, key, value)
For functions we use PEP8 naming: lower_case_with_underscores.
+# Generate apt-ftparchive extra overrides for Task fields.
+for seedname in structure.names:
+if seedname == 'extra':
+continue
This nested loop is too long as well. Be especially careful with “break” and
“continue” in combination with further nesting: those are particularly
sensitive to maintenance mistakes. Famously brought down ATT's worldwide
phone network once.
The break continue keywords aren't inherently evil, but surprisingly often
they're a sign that your code could usefully be expressed more clearly. They
make it easy to pile up alternate code paths, which the reader must all keep in
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad
Getting a better feel for the structure of runGerminate now. Which makes me
wonder:
+# Generate apt-ftparchive extra overrides for Build-Essential
+# fields.
+if ('build-essential' in structure.names and
+flavour == flavours[0]):
+writeOverrides('build-essential', 'Build-Essential', 'yes')
I don't suppose you could just do this at the end, outside of all loops?
if 'build-essential' in structures[flavours[0]].names:
writeOverrides('build-essential', 'Build-Essential', 'yes')
--
https://code.launchpad.net/~cjwatson/launchpad/refactor-cron-germinate/+merge/84624
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad
I think you're right that the file shouldn't be put in place if the
context exits with an exception. If I were writing this without context
managers, then rather than:
with AtomicFile('foo') as handle:
print handle, 'bar'
... I'd probably write:
handle = open('foo.new', 'w')
try:
print handle, 'bar'
finally:
handle.close()
os.rename('foo.new', 'foo')
And that's equivalent to the natural expression in (good) shell too:
set -e
echo bar foo.new
mv foo.new foo
That all suggests to me that skipping the rename on exception, and
leaving the .new file in place so that somebody with access can look at
it and find out how far the script got before it fell over, is a good
thing. I'll change the code to do that.
--
https://code.launchpad.net/~cjwatson/launchpad/refactor-cron-germinate/+merge/84624
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad
Ah, I see now why you can't do that: writeOverrides is a closure. It picks up “structure” and “arch” from the surrounding method at its point of definition. There's another easy thing to break by accident! The structure should again be structures[flavours[0]], I guess. Might it be worthwhile to separate the closure nature of writeOverrides from its functionality? If you had a separate function that takes all the necessary variables as arguments, it'd be easier to move that last paragraph out of the loop (still assuming that that makes sense at all) and it'd be easier to see that writeOverrides “borrows” some variables from runGerminate. -- https://code.launchpad.net/~cjwatson/launchpad/refactor-cron-germinate/+merge/84624 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad
On Wed, Dec 07, 2011 at 07:20:29AM -, Jeroen T. Vermeulen wrote: The part from “series = None” onward seems to be an isolated unit of work. I think it looks for the first distroseries in development state, or failing that, the first in frozen state. But the structure of the code makes that hard to figure out, and then only afterwards I can start wondering why you do it exactly this way. I fervently recommend extracting that code into a sensibly-named function. (It doesn't need to be a method: AFAICS a distribution goes in, a series comes out, and that is the full extent of its interaction with the rest of the world). Come to think of it, might there already be a suitable method in Distribution somewhere? Fair comment. Distribution.currentseries is nearly there: it just sometimes returns series in statuses we don't want here; but if there's one we can use then it will always return it, so we can just call currentseries and then check the result. * For Python strings I find consistent double quotes a good habit, at least for free-form texts that may just as well contain an apostrophe. This is an out-of-context habit of mine from Perl and shell programming: since single and double quotes have different interpolation characteristics there, I've trained myself to use single quotes unless either I have an explicit reason to want interpolation or the string contains an apostrophe and no double quotes. I realise this doesn't make as much sense for Python, so I'll go through and amend this. -- https://code.launchpad.net/~cjwatson/launchpad/refactor-cron-germinate/+merge/84624 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad
Some very nice tests there! To continue: === added file 'lib/lp/archivepublisher/tests/test_generate_extra_overrides.py' +def file_contents(path): +Return the contents of the file at path. +with open(path) as handle: +return handle.read() Since you're only reading the file, there's probably no hurry to close this file handle. So you could just return “open(path).read().” -- https://code.launchpad.net/~cjwatson/launchpad/refactor-cron-germinate/+merge/84624 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad
[I've not responded to your entire review point-by-point, but I believe
I've addressed all points where I haven't given an explicit response.]
On Wed, Dec 07, 2011 at 09:22:24AM -, Jeroen T. Vermeulen wrote:
+def runGerminate(self, override_file, series_name, arch, flavours,
+ structures):
Rightly or wrongly, the “run” in that name led me to expect that this
method would fire up a separate process. Maybe a very short docstring
to dispel that notion?
The name doesn't actually make a lot of sense; I think originally I was
in the mindset where this actually did fire up a separate process. I've
renamed the script instance to germinateArch (with a docstring), and the
test instance to fetchGerminatedOverrides. Does that help?
To me, your branch is of very high quality but this is its Achilles'
heel. The following loop body is massive! I can't see it all on one
page, so it becomes hard even to see just how long it is. If I judged
the indentations right, it makes up the entire rest of the method.
That's fair; I went back and forward a bit on this, but clearly landed
in the wrong place. Actually, now that I've renamed runGerminate to
germinateArch, it's easier in my mind to turn a block of it into
germinateArchFlavour in turn. (Regularised naming may be boring, but I
like it anyway.)
The methods do start ending up with quite a few parameters, but that's
probably better than the alternative.
Once you've extracted the loop body, given its size, it's probably
still worth extracting chunks from that.
I've done a good deal of this, although will probably end up
restructuring a bit further per your later comments.
According to some, the pattern of “comment, block of code, blank line,
comment, block of code, blank line” is a strong indication that you
need to split things up. It shows that you have already done so in
your own mind, but leaves each individual reader to figure out what
the dependencies between consecutive blocks are.
I don't necessarily agree with this in all cases (e.g. I don't know that
the new writeGerminateOutput method would benefit much from being split
further), but I agree that the original version of this code was rather
too far in the other direction.
+# Add this to the germinate log as well so that that can be
+# debugged more easily. Log a separator line first.
+self.germinate_logger.info('', extra={'progress': True})
+self.germinate_logger.info('Germinating for %s/%s/%s',
+ flavour, series_name, arch,
+ extra={'progress': True})
What does the “extra” parameter do?
That attaches a dictionary of extra attributes to the log message which
can then be picked up by the formatter. Germinate uses this for certain
messages (arguably a wart in germinate's design; I should probably look
into doing that more neatly at some point). I've moved this to its own
method so that it can have a clarifying docstring.
That's a lot of string manipulation. It may be clearer as a regex:
task_header_regex = re.compile(task-([^:]*):(.*), flags=re.IGNORECASE)
for line in seedtext:
match = task_header_regex.match(line)
if match is not None:
key, value = match.groups()
task_headers[key.lower()] = value.strip()
Thanks. I used this mostly verbatim, although I simplified using .*?.
If flavours[0] has a special meaning, that's worth documenting.
Consider documenting it in a docstring, or even making it a separate
parameter.
I've added a primary_flavour parameter.
Regarding your question of whether Build-Essential overrides should be
pulled out of the loop, it's a tough call, but I actually think it's
clearer inside the loop now that we have the primary_flavour parameter.
That's because otherwise I'd have to pass the results of germination out
of the germinateArchFlavour method, and I think the flow control is
clearer if I don't do that. Also, I think it's worth keeping the two
write overrides pieces next to each other.
I think for now flavours[0] is always “ubuntu,” but will we want to
generalize this code for derived distros later?
As you say. The shell code I was replacing hardcoded [ $distro =
ubuntu ] tests, but I didn't see a need to repeat that hardcoding.
Thanks for the detailed commentary on seed/task parsing here; as well as
clearer code, it's resulted in the tests being a few seconds faster, and
every little helps.
--
https://code.launchpad.net/~cjwatson/launchpad/refactor-cron-germinate/+merge/84624
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad.
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe :
[Launchpad-reviewers] [Merge] lp:~cjwatson/meta-lp-deps/python-germinate into lp:meta-lp-deps
Colin Watson has proposed merging lp:~cjwatson/meta-lp-deps/python-germinate
into lp:meta-lp-deps.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #899972 in Launchpad itself: cron.germinate is very slow
https://bugs.launchpad.net/launchpad/+bug/899972
For more details, see:
https://code.launchpad.net/~cjwatson/meta-lp-deps/python-germinate/+merge/84763
Add python-germinate as a Soyuz dependency. This change is needed in order to
land lp:~cjwatson/launchpad/refactor-cron-germinate. See bug 899972 for more
details.
Please note that you need to import germinate 2.1 from my PPA into the
Launchpad PPA before merging this branch.
https://lists.launchpad.net/launchpad-dev/msg08608.html has the details.
--
https://code.launchpad.net/~cjwatson/meta-lp-deps/python-germinate/+merge/84763
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cjwatson/meta-lp-deps/python-germinate into lp:meta-lp-deps.
=== modified file 'debian/changelog'
--- debian/changelog 2011-11-11 06:41:13 +
+++ debian/changelog 2011-12-07 13:45:42 +
@@ -1,3 +1,9 @@
+launchpad-dependencies (0.103) oneiric; urgency=low
+
+ * Add python-germinate to launchpad-soyuz-dependencies (LP: #899972).
+
+ -- Colin Watson cjwat...@ubuntu.com Wed, 07 Dec 2011 13:36:22 +
+
launchpad-dependencies (0.102) oneiric; urgency=low
* Add python-lpbuildd to launchpad-developer-dependencies, so it can
=== modified file 'debian/control'
--- debian/control 2011-11-11 06:41:13 +
+++ debian/control 2011-12-07 13:45:42 +
@@ -34,7 +34,7 @@
Package: launchpad-soyuz-dependencies
Architecture: all
Depends: launchpad-dependencies (= ${source:Version}), dpkg (= 1.15.4),
- germinate, devscripts,
+ germinate, python-germinate, devscripts,
apt-utils (= ${apt-utils-version}),
${misc:Depends}
Description: Meta-package for Launchpad Soyuz packages
___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~abentley/launchpad/fix-reset-to-default into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/fix-reset-to-default
into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #892211 in Launchpad itself: reset to default in bug listing visibility
widget doesn't work
https://bugs.launchpad.net/launchpad/+bug/892211
For more details, see:
https://code.launchpad.net/~abentley/launchpad/fix-reset-to-default/+merge/84833
= Summary =
Fix bug #892211: reset to default in bug listing visibility widget doesn't
work
== Proposed fix ==
Stop updating from cookies, since those are processed server-side.
Fix cookie removal.
== Pre-implementation notes ==
Discussed with Deryck
== Implementation details ==
The field_visibility values are calculated on the server side with due
consideration to the cookie value, so there's no need read the cookie on the
client side. On load, it will be in sync with the field_visibility values, and
afterward, we explicitly synchronize it with the current settings. So it is
reasonable to remove this functionality.
This solves bug #892211, but there was an additional problem that the cookies
were not being deleted by Reset to default. The test for this was faulty,
because it assumed that Y.Cookie.get would return for a deleted cookie,
when in fact, it returns null. The Chrom(e|ium) workaround of
Y.Cookie._setDoc({cookie: }) breaks Y.Cookie.remove, so we now use it only
for Chromium-base browsers, and we disable buglisting_display_utils_tests
== Tests ==
bin/test -t test_buglisting_utils.html
== Demo and Q/A ==
Go to a bug listing.
Click the gear icon.
Add Bug age to the list of fields.
Click the gear icon.
Choose reset to default.
The default format should be displayed.
Hit reload.
The default format should still be displayed.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/javascript/tests/test_buglisting_utils.js
lib/lp/bugs/javascript/buglisting_utils.js
--
https://code.launchpad.net/~abentley/launchpad/fix-reset-to-default/+merge/84833
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~abentley/launchpad/fix-reset-to-default into lp:launchpad.
=== modified file 'lib/lp/bugs/javascript/buglisting_utils.js'
--- lib/lp/bugs/javascript/buglisting_utils.js 2011-12-05 06:27:11 +
+++ lib/lp/bugs/javascript/buglisting_utils.js 2011-12-07 20:11:26 +
@@ -250,31 +250,6 @@
},
/**
- * Update field_visibility based on fields stored
- * in cookies. This is used as a light-weight
- * page to page persistence mechanism.
- *
- * @method updateFromCookie
- */
-updateFromCookie: function() {
-var cookie_name = this.get('cookie_name');
-var cookie_fields = Y.Cookie.getSubs(cookie_name);
-if (Y.Lang.isValue(cookie_fields)) {
-// We get true/false back as strings from Y.Cookie,
-// so we have to convert them to booleans.
-Y.each(cookie_fields, function(val, key, obj) {
-if (val === 'true') {
-val = true;
-} else {
-val = false;
-}
-obj[key] = val;
-});
-this.updateFieldVisibilty(cookie_fields);
-}
-},
-
-/**
* Set the given value for the buglisting config cookie.
* If config is not specified, the cookie will be cleared.
*
@@ -287,7 +262,7 @@
path: '/',
expires: new Date('January 19, 2038')});
} else {
-Y.Cookie.remove(cookie_name);
+Y.Cookie.remove(cookie_name, {path: '/'});
}
},
@@ -298,7 +273,6 @@
* @method _extraRenderUI
*/
_extraRenderUI: function() {
-this.updateFromCookie();
var form_content = this.buildFormContent();
var on_submit_callback = Y.bind(this.handleOverlaySubmit, this);
util_overlay = new Y.lazr.FormOverlay({
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting_utils.js'
--- lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-11-28 21:28:44 +
+++ lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-12-07 20:11:26 +
@@ -10,10 +10,19 @@
var ArrayAssert = Y.ArrayAssert;
var ObjectAssert = Y.ObjectAssert;
-suite.add(new Y.Test.Case({
+
+var running_in_chrome = function(){
+return (/Chrome/).test(navigator.userAgent);
+};
+
+
+var buglisting_display_utils_tests = new Y.Test.Case({
name: 'buglisting_display_utils_tests',
+_should: {ignore: []},
+
+
setUp: function() {
// Default values for model config.
this.defaults = {
@@ -48,7 +57,9 @@
}
};
// _setDoc is required for tests using
[Launchpad-reviewers] [Merge] lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad. Requested reviews: Launchpad code reviewers (launchpad-reviewers) Related bugs: Bug #901332 in Launchpad itself: AttributeError: 'SourcePackage' object has no attribute 'personHasDriverRights' nominating a bug https://bugs.launchpad.net/launchpad/+bug/901332 For more details, see: https://code.launchpad.net/~sinzui/launchpad/nomminate-driver-permissions-1/+merge/84850 Allow driver permission checks on source packages. Launchpad bug: https://bugs.launchpad.net/bugs/901332 Pre-implementation: No one SourcePackage does implement all of the IHasDrivers interface. The personHasDriverRights that is used for permission checks to work with the object is missing. It is not possible to nominate a fix for a source package at the moment because a regression caused to an effort to implement a single consistent set of permission rules in the bugs domain. RULES * personHasDriverRights on SourcePackage, consider using the HasDriversMixin * ADDENDUM: DSP uses HasDriversMixin to implement personHasDriverRights, but permissions are not specified for callsites to use it. QA * Visit https://bugs.qastaging.launchpad.net/ubuntu/oneiric/+source/linux/+bug/861296/+nominate * Verify the page displays (The page will probably say your do not have permission to nominate) * Visit https://bugs.qastaging.launchpad.net/ubuntu/+source/linux/+bug/861296/+nominate * Verify the page displays (The page will probably say your do not have permission to nominate LINT lib/lp/registry/configure.zcml lib/lp/registry/model/sourcepackage.py lib/lp/registry/tests/test_distributionsourcepackage.py lib/lp/registry/tests/test_sourcepackage.py TEST ./bin/test -vvc -t river lp.registry.tests.test_sourcepackage ./bin/test -vvc -t river lp.registry.tests.test_distributionsourcepackage IMPLEMENTATION Added the HasDriversMixin to SourcePackage to complete the implementation of IHasDrivers. lib/lp/registry/model/sourcepackage.py lib/lp/registry/tests/test_sourcepackage.py Added personHasDriverRights to ZCML after I discovered that adding a comparable test for personHasDriverRights did errored even though the methods is defined. lib/lp/registry/configure.zcml lib/lp/registry/tests/test_distributionsourcepackage.py -- https://code.launchpad.net/~sinzui/launchpad/nomminate-driver-permissions-1/+merge/84850 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad. === modified file 'lib/lp/registry/configure.zcml' --- lib/lp/registry/configure.zcml 2011-11-28 03:51:37 + +++ lib/lp/registry/configure.zcml 2011-12-07 21:41:27 + @@ -487,6 +487,7 @@ latest_overall_publication name official_bug_tags +personHasDriverRights publishing_history releases sourcepackagename === modified file 'lib/lp/registry/model/sourcepackage.py' --- lib/lp/registry/model/sourcepackage.py 2011-11-23 03:09:55 + +++ lib/lp/registry/model/sourcepackage.py 2011-12-07 21:41:27 + @@ -66,6 +66,7 @@ ISourcePackage, ISourcePackageFactory, ) +from lp.registry.model.hasdrivers import HasDriversMixin from lp.registry.model.packaging import Packaging from lp.registry.model.suitesourcepackage import SuiteSourcePackage from lp.soyuz.enums import ( @@ -195,7 +196,8 @@ class SourcePackage(BugTargetBase, HasBugHeatMixin, HasCodeImportsMixin, HasTranslationImportsMixin, HasTranslationTemplatesMixin, -HasBranchesMixin, HasMergeProposalsMixin): +HasBranchesMixin, HasMergeProposalsMixin, +HasDriversMixin): A source package, e.g. apache2, in a distroseries. This object is not a true database object, but rather attempts to === modified file 'lib/lp/registry/tests/test_distributionsourcepackage.py' --- lib/lp/registry/tests/test_distributionsourcepackage.py 2011-11-23 03:43:53 + +++ lib/lp/registry/tests/test_distributionsourcepackage.py 2011-12-07 21:41:27 + @@ -153,6 +153,14 @@ self.assertNotEqual([], distribution.drivers) self.assertEqual(dsp.drivers, distribution.drivers) +def test_personHasDriverRights(self): +# A distribution driver has driver permissions on a DSP. +distribution = self.factory.makeDistribution() +dsp = self.factory.makeDistributionSourcePackage( +distribution=distribution) +driver = distribution.drivers[0] +self.assertTrue(dsp.personHasDriverRights(driver)) + class TestDistributionSourcePackageFindRelatedArchives(TestCaseWithFactory): === modified file
[Launchpad-reviewers] [Merge] lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad
The proposal to merge lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad has been updated. Description changed to: Allow driver permission checks on source packages. Launchpad bug: https://bugs.launchpad.net/bugs/901332 Pre-implementation: No one SourcePackage does not implement all of the IHasDrivers interface. The personHasDriverRights that is used for permission checks to work with the object is missing. It is not possible to nominate a fix for a source package at the moment because a regression caused to an effort to implement a single consistent set of permission rules in the bugs domain. RULES * Add personHasDriverRights on SourcePackage, consider using the HasDriversMixin * ADDENDUM: DSP uses HasDriversMixin to implement personHasDriverRights, but permissions are not specified for callsites to use it. QA * Visit https://bugs.qastaging.launchpad.net/ubuntu/oneiric/+source/linux/+bug/861296/+nominate * Verify the page displays (The page will probably say your do not have permission to nominate) * Visit https://bugs.qastaging.launchpad.net/ubuntu/+source/linux/+bug/861296/+nominate * Verify the page displays (The page will probably say your do not have permission to nominate LINT lib/lp/registry/configure.zcml lib/lp/registry/model/sourcepackage.py lib/lp/registry/tests/test_distributionsourcepackage.py lib/lp/registry/tests/test_sourcepackage.py TEST ./bin/test -vvc -t river lp.registry.tests.test_sourcepackage ./bin/test -vvc -t river lp.registry.tests.test_distributionsourcepackage IMPLEMENTATION Added the HasDriversMixin to SourcePackage to complete the implementation of IHasDrivers. lib/lp/registry/model/sourcepackage.py lib/lp/registry/tests/test_sourcepackage.py Added personHasDriverRights to ZCML after I discovered that adding a comparable test for personHasDriverRights did errored even though the methods is defined. lib/lp/registry/configure.zcml lib/lp/registry/tests/test_distributionsourcepackage.py For more details, see: https://code.launchpad.net/~sinzui/launchpad/nomminate-driver-permissions-1/+merge/84850 -- https://code.launchpad.net/~sinzui/launchpad/nomminate-driver-permissions-1/+merge/84850 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~mbp/launchpad/featurefixture-from-request into lp:launchpad
Martin Pool has proposed merging lp:~mbp/launchpad/featurefixture-from-request
into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~mbp/launchpad/featurefixture-from-request/+merge/84887
At the moment the test FeatureFixture assumes all scopes are active, which is
probably not what you want, and at least not what I would have expected.
This makes it look at the current request instead, but leaves an option to get
back the old behaviour, which seems not trivial to remove from the beta ribbon
tests.
--
https://code.launchpad.net/~mbp/launchpad/featurefixture-from-request/+merge/84887
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~mbp/launchpad/featurefixture-from-request into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/tests/test_publisher.py'
--- lib/canonical/launchpad/webapp/tests/test_publisher.py 2011-11-17 17:03:52 +
+++ lib/canonical/launchpad/webapp/tests/test_publisher.py 2011-12-08 04:14:24 +
@@ -170,7 +170,8 @@
u'priority': 0,
u'value': u'on',
},
-)))
+),
+override_scope_lookup=lambda scope_name: True))
request = LaunchpadTestRequest()
view = LaunchpadView(object(), request)
view.related_features = ['test_feature']
@@ -207,7 +208,8 @@
# but have different values,
# then the property related_feature_info contains this feature flag.
self.useFixture(FeatureFixture(
-{}, self.makeFeatureFlagDictionaries(u'', u'on')))
+{}, self.makeFeatureFlagDictionaries(u'', u'on'),
+override_scope_lookup=lambda scope_name: True))
request = LaunchpadTestRequest()
view = LaunchpadView(object(), request)
view.related_features = ['test_feature']
@@ -228,7 +230,8 @@
# and have the same values,
# then is_beta is false.
self.useFixture(FeatureFixture(
-{}, self.makeFeatureFlagDictionaries(u'on', u'on')))
+{}, self.makeFeatureFlagDictionaries(u'on', u'on'),
+override_scope_lookup=lambda scope_name: True))
request = LaunchpadTestRequest()
view = LaunchpadView(object(), request)
view.related_features = ['test_feature']
@@ -245,7 +248,8 @@
related_features = ['test_feature']
self.useFixture(FeatureFixture(
-{}, self.makeFeatureFlagDictionaries(u'', u'on')))
+{}, self.makeFeatureFlagDictionaries(u'', u'on'),
+override_scope_lookup=lambda scope_name: True))
request = LaunchpadTestRequest()
view = TestView(object(), request)
with person_logged_in(self.factory.makePerson()):
@@ -269,7 +273,8 @@
related_features = ['test_feature_2']
self.useFixture(FeatureFixture(
-{}, self.makeFeatureFlagDictionaries(u'', u'on')))
+{}, self.makeFeatureFlagDictionaries(u'', u'on'),
+override_scope_lookup=lambda scope_name: True))
request = LaunchpadTestRequest()
view = TestView(object(), request)
TestView2(object(), request)
=== modified file 'lib/lp/services/features/testing.py'
--- lib/lp/services/features/testing.py 2011-10-27 15:04:26 +
+++ lib/lp/services/features/testing.py 2011-12-08 04:14:24 +
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010,2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
Helpers for writing tests that use feature flags.
@@ -9,6 +9,8 @@
from fixtures import Fixture
+from lazr.restful.utils import get_current_browser_request
+
from lp.services.features import (
get_relevant_feature_controller,
install_feature_controller,
@@ -18,6 +20,9 @@
Rule,
StormFeatureRuleSource,
)
+from lp.services.features.scopes import (
+ScopesFromRequest,
+)
class FeatureFixture(Fixture):
@@ -37,14 +42,19 @@
The values are restored when the block exits.
-def __init__(self, features_dict, full_feature_rules=None):
+def __init__(self, features_dict, full_feature_rules=None,
+override_scope_lookup=None):
Constructor.
:param features_dict: A dictionary-like object with keys and values
that are flag names and those flags' settings.
+:param override_scope_lookup: If non-None, an argument that takes
+a scope name and returns True if it matches. If not specified,
+scopes are looked up from the current request.
self.desired_features = features_dict
self.full_feature_rules = full_feature_rules
+self.override_scope_lookup = override_scope_lookup
def setUp(self):
Set the feature
[Launchpad-reviewers] [Merge] lp:~wgrant/launchpad/drop-unused-views into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/drop-unused-views into lp:launchpad. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~wgrant/launchpad/drop-unused-views/+merge/84888 There are lots of unused views and templates. Delete some of them. The only really notable thing here is +teamhierarchy, which is not linked from anywhere but was accessed a grand total of once last month. -- https://code.launchpad.net/~wgrant/launchpad/drop-unused-views/+merge/84888 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/drop-unused-views into lp:launchpad. === modified file 'lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt' --- lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt 2011-09-29 05:41:31 + +++ lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt 2011-12-08 04:15:38 + @@ -355,7 +355,6 @@ This is for a team: - check(/~name18/+teamhierarchy) check(/~name18/+edit, auth=True) check(/~name18/+members) check_redirect(/~name18/+projects, status=301) === removed file 'lib/canonical/launchpad/templates/logintoken-claimprofile.pt' --- lib/canonical/launchpad/templates/logintoken-claimprofile.pt 2009-09-16 20:51:57 + +++ lib/canonical/launchpad/templates/logintoken-claimprofile.pt 1970-01-01 00:00:00 + @@ -1,19 +0,0 @@ -html - xmlns=http://www.w3.org/1999/xhtml; - xmlns:tal=http://xml.zope.org/namespaces/tal; - xmlns:metal=http://xml.zope.org/namespaces/metal; - xmlns:i18n=http://xml.zope.org/namespaces/i18n; - metal:use-macro=view/macro:page/locationless - i18n:domain=launchpad - -body - - div metal:fill-slot=main - -div metal:use-macro=context/@@launchpad_form/form / - - /div -/body - -/html - === removed file 'lib/canonical/launchpad/webapp/templates/launchpad-model.pt' --- lib/canonical/launchpad/webapp/templates/launchpad-model.pt 2011-07-27 15:24:03 + +++ lib/canonical/launchpad/webapp/templates/launchpad-model.pt 1970-01-01 00:00:00 + @@ -1,13 +0,0 @@ -div - xmlns=http://www.w3.org/1999/xhtml; - xmlns:tal=http://xml.zope.org/namespaces/tal; - xmlns:metal=http://xml.zope.org/namespaces/metal; - xmlns:i18n=http://xml.zope.org/namespaces/i18n; - xml:lang=en - lang=en - dir=ltr - i18n:domain=launchpad - - tal:json replace=structure context/getCacheJSON / - -/div === modified file 'lib/lp/blueprints/browser/configure.zcml' --- lib/lp/blueprints/browser/configure.zcml 2011-11-27 01:16:55 + +++ lib/lp/blueprints/browser/configure.zcml 2011-12-08 04:15:38 + @@ -263,9 +263,6 @@ for=lp.blueprints.interfaces.specification.ISpecification permission=zope.Public browser:page -name=+listing-column -template=../templates/specification-listing-column.pt/ -browser:page name=+listing-simple template=../templates/specification-listing-simple.pt/ browser:page === removed file 'lib/lp/blueprints/templates/specification-listing-column.pt' --- lib/lp/blueprints/templates/specification-listing-column.pt 2009-07-17 17:59:07 + +++ lib/lp/blueprints/templates/specification-listing-column.pt 1970-01-01 00:00:00 + @@ -1,13 +0,0 @@ -tal:root - xmlns:tal=http://xml.zope.org/namespaces/tal; - xmlns:metal=http://xml.zope.org/namespaces/metal; - xmlns:i18n=http://xml.zope.org/namespaces/i18n; - omit-tag= - -img src=/@@/blueprint tal:replace=structure context/image:icon / -a tal:content=context/name - tal:attributes=href context/fmt:url; title context/title - spec-listing-name/a for -a tal:content=context/target/name - tal:attributes=href context/target/fmt:urlubuntu/a -/tal:root === modified file 'lib/lp/bugs/browser/bugtask.py' --- lib/lp/bugs/browser/bugtask.py 2011-12-07 04:56:36 + +++ lib/lp/bugs/browser/bugtask.py 2011-12-08 04:15:38 + @@ -24,7 +24,6 @@ 'BugTaskListingItem', 'BugTaskListingView', 'BugTaskNavigation', -'BugTaskPortletView', 'BugTaskPrivacyAdapter', 'BugTaskRemoveQuestionView', 'BugTasksAndNominationsView', @@ -1189,20 +1188,6 @@ self.next_offset (self.total_comments + self.total_activity)) -class BugTaskPortletView: -A portlet for displaying a bug's bugtasks. - -def alsoReportedIn(self): -Return a list of IUpstreamBugTasks in which this bug is reported. - -If self.context is an IUpstreamBugTasks, it will be excluded -from this list. - -return [ -task for task in self.context.bug.bugtasks -if task.id is not self.context.id] - - def get_prefix(bugtask): Return a prefix that can be used for this form. === modified file 'lib/lp/bugs/browser/configure.zcml' --- lib/lp/bugs/browser/configure.zcml 2011-12-07 04:56:36 + +++ lib/lp/bugs/browser/configure.zcml 2011-12-08 04:15:38 + @@ -18,12 +18,6
[Launchpad-reviewers] [Merge] lp:~wgrant/launchpad/drop-unused-views into lp:launchpad
The proposal to merge lp:~wgrant/launchpad/drop-unused-views into lp:launchpad has been updated. Commit Message changed to: Delete various unused views and templates. For more details, see: https://code.launchpad.net/~wgrant/launchpad/drop-unused-views/+merge/84888 -- https://code.launchpad.net/~wgrant/launchpad/drop-unused-views/+merge/84888 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/drop-unused-views into lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~lifeless/python-oops-tools/prune into lp:python-oops-tools
Robert Collins has proposed merging lp:~lifeless/python-oops-tools/prune into lp:python-oops-tools. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~lifeless/python-oops-tools/prune/+merge/84892 Pruning too many oopses at once makes django orm sad. -- https://code.launchpad.net/~lifeless/python-oops-tools/prune/+merge/84892 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-tools/prune into lp:python-oops-tools. === modified file 'src/oopstools/oops/models.py' --- src/oopstools/oops/models.py 2011-11-21 04:50:37 + +++ src/oopstools/oops/models.py 2011-12-08 04:59:25 + @@ -129,14 +129,20 @@ def prune_unreferenced(klass, prune_from, prune_until, references): # XXX: This trusts date more than we may want to - should consider # having a date_received separate to the date generated. -to_delete = set(Oops.objects.filter( -date__gte=prune_from, date__lte=prune_until).exclude( -oopsid__in=references)) -# deleting 1 at a time is a lot of commits, but leaves the DB lively -# for other transactions. May need to batch this in future if its -# too slow. -for oopsid in to_delete: -Oops.objects.filter(oopsid__exact=oopsid).delete() +while True: +# There may be very many OOPS to prune, so only ask for a few at a +# time. This prevents running out of memory in the prune process +# (can happen when millions of reports are being pruned at once). +to_delete = set(Oops.objects.filter( +date__gte=prune_from, date__lte=prune_until).exclude( +oopsid__in=references)[:1]) +# deleting 1 at a time is a lot of commits, but leaves the DB lively +# for other transactions. May need to batch this in future if its +# too slow. +for oopsid in to_delete: +Oops.objects.filter(oopsid__exact=oopsid).delete() +if not to_delete: +break class Report(models.Model): ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~mbp/launchpad/show-timeline into lp:launchpad
The proposal to merge lp:~mbp/launchpad/show-timeline into lp:launchpad has been updated. Status: Needs review = Approved For more details, see: https://code.launchpad.net/~mbp/launchpad/show-timeline/+merge/80166 -- https://code.launchpad.net/~mbp/launchpad/show-timeline/+merge/80166 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/show-timeline into lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
