SVN completed #179

Merged
merged 1 commit into from Jul 27, 2011

Conversation

Projects
None yet
3 participants
@in3xes
Contributor

in3xes commented Jul 11, 2011

Please take closer look than usual, especially purge and url stuff.

@djmitche

View changes

master/docs/cfg-buildsteps.texinfo
+
+For added flexibility, @code{baseURL} may contain a @code{%%BRANCH%%}
+placeholder, which will be replaced either by the branch in the SourceStamp or
+the default specified in @code{defaultBranch}.

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

This should be described in the @item for baseURL, above!

@djmitche

djmitche Jul 20, 2011

Member

This should be described in the @item for baseURL, above!

@djmitche

View changes

master/docs/cfg-buildsteps.texinfo
+@item clobber
+It removes the working directory for each build then makes full checkout.
+@item fresh
+This is alternative for incremental (old update) mode with @code{always_purge}

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

Better phrased as "This is equivalent to the old update mode with @code{always_purge}", and only mentioned as the last sentence in this @item. That is:

This always always purges local changes before updating. This deletes
unversioned files and reverts everything that would appear in a @code{svn status
--no-ignore}. This is equivalent to the old update mode with @code{always_purge}.

@djmitche

djmitche Jul 20, 2011

Member

Better phrased as "This is equivalent to the old update mode with @code{always_purge}", and only mentioned as the last sentence in this @item. That is:

This always always purges local changes before updating. This deletes
unversioned files and reverts everything that would appear in a @code{svn status
--no-ignore}. This is equivalent to the old update mode with @code{always_purge}.

+ self.expectOutcome(result=SUCCESS, status_text=["update"])
+ return self.runStep()
+
+class TestGetUnversionedFiles(unittest.TestCase):

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

I'd like to see some more tests here - invalid XML, an XML document with no entries, an entry with no wc-status, a status with no item, and a wc-status with no path, at least -- just to see how those failures are handled.

@djmitche

djmitche Jul 20, 2011

Member

I'd like to see some more tests here - invalid XML, an XML document with no entries, an entry with no wc-status, a status with no item, and a wc-status with no path, at least -- just to see how those failures are handled.

@djmitche

View changes

master/buildbot/steps/source/svn.py
+ filename = entry.getAttribute('path')
+ if filename in keep_on_purge:
+ continue
+ yield filename

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

Nice use of a generator :)

@djmitche

djmitche Jul 20, 2011

Member

Nice use of a generator :)

This comment has been minimized.

@in3xes

in3xes Jul 22, 2011

Contributor

I copied that from slaveside ;)

@in3xes

in3xes Jul 22, 2011

Contributor

I copied that from slaveside ;)

@djmitche

View changes

master/buildbot/steps/source/svn.py
+
+ @staticmethod
+ def getUnversionedFiles(xmlStr, keep_on_purge):
+ """Delete everything that shown up on status."""

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

This comment isn't quite right.. this method doesn't delete anything!

@djmitche

djmitche Jul 20, 2011

Member

This comment isn't quite right.. this method doesn't delete anything!

+ c.addCallback(evaluateCommand)
+ return c
+
+ @staticmethod

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

Why static?

@djmitche

djmitche Jul 20, 2011

Member

Why static?

This comment has been minimized.

@in3xes

in3xes Jul 22, 2011

Contributor

Only for tests, no other reason.

@in3xes

in3xes Jul 22, 2011

Contributor

Only for tests, no other reason.

+ c = self._dovccmd(command)
+ def parseAndRemove(_):
+ output = self.getLog('stdio').getText()
+ output = output[output.find('<'):]

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

I'm not sure what this is about - finding the first tag? Why?

@djmitche

djmitche Jul 20, 2011

Member

I'm not sure what this is about - finding the first tag? Why?

This comment has been minimized.

@in3xes

in3xes Jul 22, 2011

Contributor

When I do self.getLog('stdio').getText() it gives the output previous commands concatenated to one after other. So, I am find first '<' to find the starting of the xml data. Any other way?

@in3xes

in3xes Jul 22, 2011

Contributor

When I do self.getLog('stdio').getText() it gives the output previous commands concatenated to one after other. So, I am find first '<' to find the starting of the xml data. Any other way?

@djmitche

View changes

master/buildbot/steps/source/svn.py
+ " baseURL")
+
+ self.svnurl = self.svnurl and _ComputeRepositoryURL(self.svnurl)
+ self.baseURL = self.baseURL and _ComputeRepositoryURL(self.baseURL)

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

Since you submitted this pull req, @tomprince has removed the _ComputeRepositoryURL invocations from new source steps, so please remove them here, too.

@djmitche

djmitche Jul 20, 2011

Member

Since you submitted this pull req, @tomprince has removed the _ComputeRepositoryURL invocations from new source steps, so please remove them here, too.

@djmitche

View changes

master/buildbot/steps/source/svn.py
+ self.keep_on_purge = keep_on_purge or []
+ self.depth = depth
+ self.method=method
+ Source.__init__(self, **kwargs)

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

This is an unusual place to call the parent constructor. It should either be called before the other attributes are set, or after all of them are set.

@djmitche

djmitche Jul 20, 2011

Member

This is an unusual place to call the parent constructor. It should either be called before the other attributes are set, or after all of them are set.

@djmitche

View changes

master/buildbot/steps/source/svn.py
+ method=None, defaultBranch=None, username=None,
+ password=None, extra_args=None, keep_on_purge=None,
+ depth=None, **kwargs):
+ """

This comment has been minimized.

@djmitche

djmitche Jul 20, 2011

Member

This docstring duplicates information available in the manual, and in fact doesn't even cover all of the constructor arguments. I think it should be removed.

@djmitche

djmitche Jul 20, 2011

Member

This docstring duplicates information available in the manual, and in fact doesn't even cover all of the constructor arguments. I think it should be removed.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jul 20, 2011

Member

A few points:

baseURL vs svnurl

These names are confusing, and as I understand it, are just a vestige of backward compatibility. Since this step requires that users choose it explicitly, we have a chance to do it right. What do these two parameters accomplish, and how could we do that with one parameter, with lots of flexibility for the user? What about the common case when the repository in the SourceStamp is sufficient - do we need to make the user specify a URL to the SVN step in that case, too? You don't need to implement the full flexibility yet, but think ahead when deciding how to handle these URLs.

Tests on master

I pulled this onto master, and while it merged smoothly, the tests did not pass after the merge. This is probably "bitrot" after it took me so long to review - sorry.. Can you take a look?

Member

djmitche commented Jul 20, 2011

A few points:

baseURL vs svnurl

These names are confusing, and as I understand it, are just a vestige of backward compatibility. Since this step requires that users choose it explicitly, we have a chance to do it right. What do these two parameters accomplish, and how could we do that with one parameter, with lots of flexibility for the user? What about the common case when the repository in the SourceStamp is sufficient - do we need to make the user specify a URL to the SVN step in that case, too? You don't need to implement the full flexibility yet, but think ahead when deciding how to handle these URLs.

Tests on master

I pulled this onto master, and while it merged smoothly, the tests did not pass after the merge. This is probably "bitrot" after it took me so long to review - sorry.. Can you take a look?

@tomprince

This comment has been minimized.

Show comment
Hide comment
@tomprince

tomprince Jul 22, 2011

Member

On Wed, 20 Jul 2011 00:24:04 -0700, djmitche reply@reply.github.com wrote:

A few points:

baseURL vs svnurl

These names are confusing, and as I understand it, are just a vestige
of backward compatibility. Since this step requires that users choose
it explicitly, we have a chance to do it right. What do these two
parameters accomplish, and how could we do that with one parameter,
with lots of flexibility for the user? What about the common case
when the repository in the SourceStamp is sufficient - do we need to
make the user specify a URL to the SVN step in that case, too? You
don't need to implement the full flexibility yet, but think ahead when
deciding how to handle these URLs.

This comment also applies to repoURL/baseURL of the Mercurial step. It
would be good if all the new source steps could have consistent
paramater naming among them selves, rather than with the old steps,
which were not consistent.

Tom

Member

tomprince commented Jul 22, 2011

On Wed, 20 Jul 2011 00:24:04 -0700, djmitche reply@reply.github.com wrote:

A few points:

baseURL vs svnurl

These names are confusing, and as I understand it, are just a vestige
of backward compatibility. Since this step requires that users choose
it explicitly, we have a chance to do it right. What do these two
parameters accomplish, and how could we do that with one parameter,
with lots of flexibility for the user? What about the common case
when the repository in the SourceStamp is sufficient - do we need to
make the user specify a URL to the SVN step in that case, too? You
don't need to implement the full flexibility yet, but think ahead when
deciding how to handle these URLs.

This comment also applies to repoURL/baseURL of the Mercurial step. It
would be good if all the new source steps could have consistent
paramater naming among them selves, rather than with the old steps,
which were not consistent.

Tom

@in3xes

This comment has been minimized.

Show comment
Hide comment
@in3xes

in3xes Jul 23, 2011

Contributor

Updated, will fix URL stuff later.

Contributor

in3xes commented Jul 23, 2011

Updated, will fix URL stuff later.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jul 27, 2011

Member

Merged (finally), yay!

Member

djmitche commented Jul 27, 2011

Merged (finally), yay!

@djmitche djmitche merged commit a31ef36 into buildbot:master Jul 27, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment