Skip to main content

[pkg-discuss] Re: Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher

  • From: Xiaobo Shen < >
  • To: Tim Foster < >
  • Cc:
  • Subject: [pkg-discuss] Re: Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher
  • Date: Wed, 28 Aug 2013 13:59:05 -0700

On 08/27/13 07:52 PM, Tim Foster wrote:
Hi there,

On 08/28/13 01:03 PM, Xiaobo Shen wrote:
please see the following link:
http://ips.java.net/webrev/xiaoshen/Bug_15771544_pkgrepo2/

I had a look at this. The webrev looks strange - I assume there's multiple changesets in your workspace and you just haven't recommitted yet (hence the duplicated webrev comments)

You can get around that in the future by running /opt/onbld/bin/hg-active, saving the output to a file, then editing the file, then passing that to webrev with the '-w' flag.


Generally, I'm not wild about the approach of pkgrepo(1) doing an 'rm -rf' in the background: if the system is rebooted or crashes while that command is still running, on reboot we'll be left with a pile of orphaned content in the repository root tmp dir.

I'd prefer to have a "--synchronous" flag to pkgrepo(1) (I'm not sure if there's precedent for a short-option to use, -s would be the obvious candidate if pkg(5) didn't use that already)

Using that option on the command line would have pkgrepo(1) wait for the command to complete. If we decide that option isn't necessary, then I think it's important to *very* clearly document the fact that this command starts a background task.


Some specific commands on the code:

src/man/pkgrepo.1

The changes here could probably use some word-smithing. Alta might be able to help with that.


src/modules/server/repository.py

line 2910, please use capitalization and a period at the end of the sentence.

line 2911, pfxs must be an iterable, so we should state that here.

line 2950, so far, we only have one purpose for this method and the tempdir is named to indicate that purpose so it's probably better to leave the docstring as:

"""Create a temporary directory beneath the repository root or writable root."""


src/pkgrepo.py

line 356, please use capitalization and period at the end of the sentence, and add blank line after the docstring.

line 397, there's two spaces in the message "not  exist"

line 407, there should be spaces between each dictionary key and value, eg. {"key": "value"} not {"key":"value"}

line 415, this sentence seems clunky. How about:

"# Change the repository publisher/prefix property, if necessary."

line 419, you need a space after the variable name and before the operator in "rval="

line 419, 425 there should only be one space after the '='

line 420, you only need 4 spaces for follow-on indents

line 421, 422: just a nit - I prefer to see whitespace at the end of string literals that end on one line and continue on another, rather than having a string literal starting with a whitespace on the continuation line.

line 421, since we can remove multiple publishers in a single operation, this message doesn't read very well ("The publisher being removed.." doesn't allow for plurals) How about:

"The default publisher was removed. Setting 'publisher/prefix' to %s"

and later on, at line 427

"The default publisher was removed. The 'publisher/prefix' property should be set to one of the remaining publishers: %s"



src/tests/cli/t_pkgrepo.py

line 2165, should be "Verify that the remove-publisher subcommand works as expected."""

line 2179, "nol publisher" ?

line 2183, this comment doesn't apply to the code that it seems to reference. Just removing the comment would be fine.

line 2245, better would be "...if all publishers do not exist."

line 2277, better might be "Verify that if one publisher is removed and there is more than one publisher left, and one of the removed publishers was the default publisher, that we unset the publisher/prefix property."

line 2294, 2295 there should be a space after the '#' character

line 2297, you shouldn't be creating directories in '/tmp' because then tests run in parallel can step on each others toes, or we get different results on different test machines (for example, systems where "/tmp/randomrepo" exists as a file, or some other unrelated test decides to use that path as well) If we assert anywhere from here on, we won't run the cleanup code at line 2306. Instead, please use self.test_root

line 2299, 2305, there should not be spaces in parameter values being passed to a method. Use 'su_wrap=True' instead.

line 2300, 2303 this seems weird. If the testsuite is already running as root, then you don't need line 2303 surely?


That's all I could come up with for now,

    cheers,
            tim


Thanks a lot for your comments. I have fixed most of the issues. For the "rm -rf" subprocess, I am unsure if we can really avoid pub folder left in the tmp folder by just using synchronous flag. I guess during the system crashes, it is still possible to have something uncleaned in tmp folder.

xiaobo


[pkg-discuss] Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher

Xiaobo Shen 08/28/2013

[pkg-discuss] Re: Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher

Tim Foster 08/28/2013

[pkg-discuss] Re: Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher

Xiaobo Shen 08/28/2013

[pkg-discuss] Re: Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher

Tim Foster 08/28/2013

[pkg-discuss] Re: Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher

Erik Trauschke 08/28/2013

[pkg-discuss] Re: Review request 2 for 15771544 pkgrepo should have a way of removing an entire publisher

Xiaobo Shen 08/29/2013
 
 
Close
loading
Please Confirm
Close