> > http://cr.opensolaris.org/~johansen/pkg-mirror1/
>
> http://cr.opensolaris.org/~johansen/pkg-mirror1/src/client.py.wdiff.html
> ==========
> 105 + [-O origin_url] [-m mirror to add] [-M mirror to
> remove] authority
>
> I don't think -m and -M are very obvious -- they don't say to me as a
> user, "add and remove." Specifically, it would be too easy to say -M
> when I met -m or vice-versa.
>
> Could we use --add-mirror and --remove-mirror instead or some other set
> of letters like -a and -r?
I had a completely separate set of commands that you could use to add or
remove a mirror. I had run the idea by Dan and Danek, and they were
fine with it. However, Stephen objected to that usage and wanted these
to be a subcommand of set-authority. I don't agree, but he was pretty
firm about that. You'll need to take this up with him.
> 1622 1658 except misc.TransferTimedOutException:
> 1623 1659 msg(_("Maximum number of timeouts exceeded
> during download."))
> 1624 1660 sys.exit(1)
> 1661 + except misc.InvalidContentException, e:
> 1662 + msg(_("One or more hosts providing content
> for this install " +
> 1663 + "has provided a file with invalid
> content."))
> 1664 + msg(_(str(e)))
> 1665 + sys.exit(1)
>
>
> Would it be better to use emsg() for these instead of msg() so that they
> go to stderr? What is our rule for using stderr vs. stdout?
I switched to error(). It's a wrapper around emsg(). No idea what the
rules are. Rules aren't any fun anyway.
> http://cr.opensolaris.org/~johansen/pkg-mirror1/src/depot.py.wdiff.html
> ==========
> 66 +MIRROR_DEFAULT = False
>
> Can you add a comment to be consistent with the other default constants?
Yes. I can and I shall.
> 93 +Usage: /usr/lib/pkg.depotd [--readonly] [--rebuild]
> [--mirror] [-d repo_dir]
> 94 + [-p port] [-s threads] [-t socket_timeout]
>
> Since you are here...
>
> Is it really appropriate for us to place /usr/lib in the text here given
> our multi-platform nature?
It depends how nitpicky you'd like to be. Both pkg and pkg.depotd hard
code the name of the command into the usage and error output.
If you'd like to be precise, this is entirely incorrect since there's no
telling what the command was named when we invoked it. The binary (or
script, in this case) can be renamed to anything. Ideally we'd get the
name of the command that was actually invoked out of argv[].
Realistically, we don't do this anywhere else in the code we have today.
I'm inclined to think it doesn't matter much now. I would file a
bug/RFE (depends how you want to argue this) if you think this really
needs to be fixed.
> 98 + --mirror Content mirror mode; no metadata
> operations
>
>
> s/metadata operations/publishing and metadata operations disabled/ ?
Ok.
> Should mirror be allowed in combination with readonly or does it matter?
Mirror is a more restrictive subset of readonly, so I'm inclined to say
that it doesn't matter. Did you see somewhere where I might have gotten
this wrong?
> http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/filelist.py.wdiff.html
> ==========
> 398 + def _pick_mirror(self, chosen_set = None):
>
> Drop spaces around '=' for PEP8 style.
>
>
> 410 + chosen_set.add(self.ds)
>
> Newline needed after this line.
>
> 416 + remove the file and raise a
> InvalidContentException."""
>
> s/a/an/
Fixed all of these.
> http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/image.py.wdiff.html
> ==========
> 311 + def select_mirror(self, auth = None, chosen_set = None):
>
> Drop spaces around '=' for PEP8.
This makes the style within image.py inconsistent and confusing. If
we're going to switch to PEP8, we need to make all of our files meet
PEP8 style. I could understand you asking me to do this for a new file;
however, it seems like it's just going to make matters worse in this
file, where all of the optional arguments have spaces between the
equals.
> 313 + the mirrors. Pick the best one. Returns
> DepotStatus object."""
>
> s/one. Returns/one and return a/
I've made a different correction.
> 452 + def add_mirror(self, auth_name, mirror):
> 453 + """Add the mirror URLs contained in mirror to
> 454 + auth_name's list of mirrors."""
>
> Can you expand the comment to include the expected format of mirror
> since it is multiple URLs? Or was this supposed to be URL instead of URLs?
It was originally multiple mirrors in a list, but is now a single one.
I've snipped the plural here to be less confusing.
> 457 + auths[auth_name]["mirrors"].append(mirror)
>
> What about a try: except: to handle the auth_name not existing?
How would that happen? We check to make sure the authority exists in
client.py. It returns an error if it can't be found. I've updated the
error message in that part of the code to be more clear.
> http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/client/imageconfig.py.wdiff.html
> ==========
> Nitpick: Why isn't it bad_tx and good_tx instead of errors and good_tx?
I think "errors" is more descriptive.
>
> 176 + self.flush_conten_cache = \
> 177 +
> self.policies["flush-content-cache-on-success"]
>
>
> s/conten_cache/content_cache/
Thanks. Good catch.
> 220 + @staticmethod
> 221 + def read_list(str):
>
> Seem like this could go into pkg.misc.
The only place it gets used right now is imageconfig. I don't have a
problem with somebody moving it to misc.py if they want to use it in
other parts of the code.
> http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/misc.py.wdiff.html
> ==========
> 335 + str = "Action with path %s should have hash
> %s. Computed hash %s instead" % \
>
> Should this end with '.' ?
Yes, it should.
> http://cr.opensolaris.org/~johansen/pkg-mirror1/src/modules/server/repository.py.wdiff.html
> ==========
>
> 187 + raise
> cherrypy.HTTPError(httplib.NOT_FOUND,
> 188 + "Operation not supported in
> current server mode.")
>
> Shouldn't this be 501 Not Implemented?
I don't believe so.
This part is in transaction.py, just so nobody else following along gets
confused:
> 281 + fs = os.stat(opath)
> 282 + action.attrs["pkg.csize"] =
> str(fs[stat.ST_SIZE])
>
> A comment above this explaining what we're doing, or an update to the
> docstring for the containing function would be nice.
I'll add a comment here.
> ----------
> General Comments
> ----------
>
> Can we open a bug for adding a test that actually ensures a client gets
> files from the mirror? Could possibly start two depots (one in normal
> mode, one in mirror mode) and then check the depot status object for the
> mirror to see if it actually got the transaction from there.
Sure.
> Can we open a few bugs about the other topics that came up such as
> mirror synchronization, mirror authentication, etc.?
I want to confer with Stephen before I do this, but I don't see a
problem.
> I assume we'll pre-populate cfg_cache with a known set of mirrors for
> future releases?
That is the eventual idea.
Thanks for taking the time to look at this.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
