New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiproject svnpoller #443

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Jc2k
Contributor

Jc2k commented Jun 3, 2012

This patch teaches SVN Poller how to support multiple projects. This means the split_file function has to return more than just branch and path. I opted to make it return an object. This makes for an easy backwards compatibility check - if i encounter a tuple then i turn it into the new object. It also means I can mutate the object - for example, in the new split_file_project_branches example i call split_file_branches and annotate the result with the project.

It closes/invalidates pull request 198 and has these main differences to that approach:

  • Doesn't introduce a new callable - still just the split_file function.
  • Doesn't introduce the project vs projects distinction. There is no whitelist of projects. If you want to whitelist which projects the poller pays attention to, do it in your split_file function.
  • Allows you to override repository (this might be a bit of an edge case - in my setup i have it rigged up so that even though the real repo root might be https://localhost/svn/root (and that is what i poll), as far as my builder is concerned the root is https://localhost/svn/root/blah/wibble/project/)
@tomprince

This comment has been minimized.

Show comment
Hide comment
@tomprince

tomprince Jun 3, 2012

Member

Our general preference is to just use dicts, for lightweight struct-like things.

Member

tomprince commented Jun 3, 2012

Our general preference is to just use dicts, for lightweight struct-like things.

@tomprince

This comment has been minimized.

Show comment
Hide comment
@tomprince

tomprince Jun 3, 2012

Member

And I guess it is too disruptive to change the existing functions to return dicts.

Member

tomprince commented Jun 3, 2012

And I guess it is too disruptive to change the existing functions to return dicts.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Jun 3, 2012

Contributor

Bleh. I'll do whatever it takes to avoid this living in a local fork, but i find dicts quite distasteful for lightweight and fixed data structures. I'd prefer a namedtuple, did it this way to maintain 2.5 support.

Can you clarify what you mean by your 2nd comment, and do you have any other suggestions? The documentation for this is a pain and having to do it twice is plenty enough for me :)

Contributor

Jc2k commented Jun 3, 2012

Bleh. I'll do whatever it takes to avoid this living in a local fork, but i find dicts quite distasteful for lightweight and fixed data structures. I'd prefer a namedtuple, did it this way to maintain 2.5 support.

Can you clarify what you mean by your 2nd comment, and do you have any other suggestions? The documentation for this is a pain and having to do it twice is plenty enough for me :)

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jun 4, 2012

Member

i'll have to second tomprince. objects have led us to sadness with methods being added, ad-hoc interfaces, user subclasses doing silly things, and import paths sticking around for eternity. Forward compatibility is a concern, so perhaps this can be accomplished with a dict containing a version number? Then future changes can increment that number and thus retain backward compatibility.

Member

djmitche commented Jun 4, 2012

i'll have to second tomprince. objects have led us to sadness with methods being added, ad-hoc interfaces, user subclasses doing silly things, and import paths sticking around for eternity. Forward compatibility is a concern, so perhaps this can be accomplished with a dict containing a version number? Then future changes can increment that number and thus retain backward compatibility.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Jun 4, 2012

Contributor

But a dict with a weakly enforced interface and a version number seems like a more ad-hoc interface than a class. Especially one that used slots like this easily could - that would render someone incapable of assigning data to to banch instead of branch, for example.

I will obviously yield to whatever you guys say, but I wont like it :p

This isn't going to be persisted. It isnt going on a queue. Its a temporary data structure with a tiny lifespan. It feels a bit weird to need a version number? What does that protect me from?

Contributor

Jc2k commented Jun 4, 2012

But a dict with a weakly enforced interface and a version number seems like a more ad-hoc interface than a class. Especially one that used slots like this easily could - that would render someone incapable of assigning data to to banch instead of branch, for example.

I will obviously yield to whatever you guys say, but I wont like it :p

This isn't going to be persisted. It isnt going on a queue. Its a temporary data structure with a tiny lifespan. It feels a bit weird to need a version number? What does that protect me from?

@dwlocks

This comment has been minimized.

Show comment
Hide comment
@dwlocks

dwlocks Jun 4, 2012

It seems sneaky to hide a project whitelist in the split function. A single project is already an SVN specific case. Allowing multiple projects is an extension of that case and should be explicit. (ie in the init call). I would deprecate project in the future, and advocate a single item list for a single project.

To comment on overriding repository: Schedulers are project aware, and can filter on that. So overriding repository for schedulers is not (obviously) useful. The svn buildstep does not rely on the repository property of a build. You can set that to whatever you want (statically or using withProperties). Overriding repository in the poller such that the project is included in the repository URL removes the flexibility we just got by adding multiple project awareness. I don't get the utility here.

But I don't need to get it. Everyone's build has different quirks, and allowing the split_ functions to override all the parts of an incoming change will surely get used.

dwlocks commented Jun 4, 2012

It seems sneaky to hide a project whitelist in the split function. A single project is already an SVN specific case. Allowing multiple projects is an extension of that case and should be explicit. (ie in the init call). I would deprecate project in the future, and advocate a single item list for a single project.

To comment on overriding repository: Schedulers are project aware, and can filter on that. So overriding repository for schedulers is not (obviously) useful. The svn buildstep does not rely on the repository property of a build. You can set that to whatever you want (statically or using withProperties). Overriding repository in the poller such that the project is included in the repository URL removes the flexibility we just got by adding multiple project awareness. I don't get the utility here.

But I don't need to get it. Everyone's build has different quirks, and allowing the split_ functions to override all the parts of an incoming change will surely get used.

@dwlocks

This comment has been minimized.

Show comment
Hide comment
@dwlocks

dwlocks Jun 4, 2012

Lately I've felt that schedulers can handle any filtering we need. So svnpoller needs to know how to split the project out of the url, but shouldn't filter at all and doesn't need an explicit project parameter. (that's different that pull request 198) Either way, the single project parameter seems limiting to me.

dwlocks commented Jun 4, 2012

Lately I've felt that schedulers can handle any filtering we need. So svnpoller needs to know how to split the project out of the url, but shouldn't filter at all and doesn't need an explicit project parameter. (that's different that pull request 198) Either way, the single project parameter seems limiting to me.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Jun 4, 2012

Contributor

So in my case I am stuck with a single repository that is absolutely out of control. It is beyond redemption. While I can't go into specifics, it absolutely does not fit in to any standard pattern. I either have to have multiple pollers or have a very creative splitter.

I have a helper function that will configure a poller for a given "repository root" - the root might be one of these:

https://svn.example.com/svn/troublerepo/someproject.module/{trunk,branches,tags}
https://svn.example.com/svn/troublerepo/trunk/someproject/someproject.module/{trunk,branches,tags}
https://svn.example.com/svn/troublerepo/teams/teama/someproject.module/{trunk,branches,tags}

In addition I have some branches that have to be treated as "trunk only" type setups:

https://svn.example.com/svn/troublerepo/branches/some_branch/

In all the above cases the real root would obviously be https://svn.example.com/svn/troublerepo/

There are even more upsetting and terrible things going on inside the team folders that I won't go into. And while the sensible answer would be to throw it all away and start again, I can't.

So I've created a split function that essentially lets me "mount" projects on it at locations - both the {trunk, branches} kind and the just-trunk kind.

The helper function identifies the real root of the repository - this could be done by convention for example but im actually using svn as this whole thing is an otherwise generic helper. It creates a single poller for the root of my repository using the splitter i mentioned.

The splitter is actually a callable class so that i can effectively continue to register other projects with the existing poller as i iterate a master list of projects. I could also flip this round and have a class with a split instance method that i pass, but i spend too much time around Plone and its a call instead.

Any other repositories that share the same root (by the 2nd project i can "if url startswith repository root" to figure that out) will be "mounted" on the splitter for the shared poller.

When a new change arrives I loop over this list of "mount points" for a match. If i find one i apply the standard file splitter. Any change that doesnt match one of these "mounts" will lead to a "return None" - effectively a whitelist, but thats not my mental model.

Now in my case forward declaring projects in the poller would be doable with some refactoring, but it doesn't add any value for me. What have I missed?

With regards overriding repository, it's down to aesthetics. Consider:

repository: https://svn.example.com/svn/troublerepo/
branch: teams/teama/someproject.module/branches/some-feature

vs

repository: https://svn.example.com/svn/troublerepo/teams/teama/someproject.module/
branch: branches/some-feature

The SVN step doesnt care if thats not really the root of the repository. If I can override repository i can have short branch names in my source stamp. This is useful in a custom web view I have this has a list of built branches for a given project (its sort of like the console view).

Contributor

Jc2k commented Jun 4, 2012

So in my case I am stuck with a single repository that is absolutely out of control. It is beyond redemption. While I can't go into specifics, it absolutely does not fit in to any standard pattern. I either have to have multiple pollers or have a very creative splitter.

I have a helper function that will configure a poller for a given "repository root" - the root might be one of these:

https://svn.example.com/svn/troublerepo/someproject.module/{trunk,branches,tags}
https://svn.example.com/svn/troublerepo/trunk/someproject/someproject.module/{trunk,branches,tags}
https://svn.example.com/svn/troublerepo/teams/teama/someproject.module/{trunk,branches,tags}

In addition I have some branches that have to be treated as "trunk only" type setups:

https://svn.example.com/svn/troublerepo/branches/some_branch/

In all the above cases the real root would obviously be https://svn.example.com/svn/troublerepo/

There are even more upsetting and terrible things going on inside the team folders that I won't go into. And while the sensible answer would be to throw it all away and start again, I can't.

So I've created a split function that essentially lets me "mount" projects on it at locations - both the {trunk, branches} kind and the just-trunk kind.

The helper function identifies the real root of the repository - this could be done by convention for example but im actually using svn as this whole thing is an otherwise generic helper. It creates a single poller for the root of my repository using the splitter i mentioned.

The splitter is actually a callable class so that i can effectively continue to register other projects with the existing poller as i iterate a master list of projects. I could also flip this round and have a class with a split instance method that i pass, but i spend too much time around Plone and its a call instead.

Any other repositories that share the same root (by the 2nd project i can "if url startswith repository root" to figure that out) will be "mounted" on the splitter for the shared poller.

When a new change arrives I loop over this list of "mount points" for a match. If i find one i apply the standard file splitter. Any change that doesnt match one of these "mounts" will lead to a "return None" - effectively a whitelist, but thats not my mental model.

Now in my case forward declaring projects in the poller would be doable with some refactoring, but it doesn't add any value for me. What have I missed?

With regards overriding repository, it's down to aesthetics. Consider:

repository: https://svn.example.com/svn/troublerepo/
branch: teams/teama/someproject.module/branches/some-feature

vs

repository: https://svn.example.com/svn/troublerepo/teams/teama/someproject.module/
branch: branches/some-feature

The SVN step doesnt care if thats not really the root of the repository. If I can override repository i can have short branch names in my source stamp. This is useful in a custom web view I have this has a list of built branches for a given project (its sort of like the console view).

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Jun 4, 2012

Contributor

I think i'd probably agree with ripping the project parameter out (it would have to stay for now for backward compatibility), but isn't some filtering good? For example, some of the repos i have to deal with have docs/ in the root and other random things. I can't classify them with a project. I might not even be able to classify them as part of a branch.

Schedulers might be able to filter out such changes, but changes end up in the database and i don't want them in the database at all really. I'd get OCD about the pointless mess that was building up behind the scenes.

Contributor

Jc2k commented Jun 4, 2012

I think i'd probably agree with ripping the project parameter out (it would have to stay for now for backward compatibility), but isn't some filtering good? For example, some of the repos i have to deal with have docs/ in the root and other random things. I can't classify them with a project. I might not even be able to classify them as part of a branch.

Schedulers might be able to filter out such changes, but changes end up in the database and i don't want them in the database at all really. I'd get OCD about the pointless mess that was building up behind the scenes.

@tomprince

This comment has been minimized.

Show comment
Hide comment
@tomprince

tomprince Jun 5, 2012

Member

I've been trying to push to have the repository attribute of sourcestamps/changes actually be a remotely accesile URL that can be used to access the repository. I know that source steps by default ignore the repository of the source stamp, but I view that as a security measure/artifact of buildbot predating dvcs's.

Given that, I think it makes sense to include the project as part of the repository, since (not using svn) it makes sense to me that a svn url should be repository+branch.

Member

tomprince commented Jun 5, 2012

I've been trying to push to have the repository attribute of sourcestamps/changes actually be a remotely accesile URL that can be used to access the repository. I know that source steps by default ignore the repository of the source stamp, but I view that as a security measure/artifact of buildbot predating dvcs's.

Given that, I think it makes sense to include the project as part of the repository, since (not using svn) it makes sense to me that a svn url should be repository+branch.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Jun 5, 2012

Contributor

And here it is, rewritten with dicts. Same test coverage as before, still passing.

Contributor

Jc2k commented Jun 5, 2012

And here it is, rewritten with dicts. Same test coverage as before, still passing.

@tomprince

View changes

Show outdated Hide outdated master/buildbot/changes/svnpoller.py Outdated
@tomprince

View changes

Show outdated Hide outdated master/buildbot/changes/svnpoller.py Outdated
@tomprince

View changes

Show outdated Hide outdated master/buildbot/changes/svnpoller.py Outdated
@tomprince

This comment has been minimized.

Show comment
Hide comment
@tomprince

tomprince Jun 6, 2012

Member

This is looking good, although I haven't had a chance to look at the tests yet.

Member

tomprince commented Jun 6, 2012

This is looking good, although I haven't had a chance to look at the tests yet.

@dwlocks

This comment has been minimized.

Show comment
Hide comment
@dwlocks

dwlocks Jun 6, 2012

TomPrince Said:

Given that, I think it makes sense to include the project as part of the repository, since (not using svn) it makes sense > to me that a svn url should be repository+branch.

But you don't use svn, so your point of while common and logical is not entirely applicable. Svn has a concept of a remotely accessible URL separate from the repository root. Look at the output of "svn info": The full url will include project and branch in an arbitrary order, while repository root is fixed. I'd like for the checkout step to use the repository in the source stamp, but to cover more than just my and jc2k's repos, we should keep project and repository separate to allow the all users to combine them appropriately. All repos are not set up repository+project+branch. repository+branch+project is also valid.

For the svn server, all subdirs past the repository root are arbitrary and have no inherent special meaning. The fact that we add that meaning confuses things quite a bit :-(

dwlocks commented Jun 6, 2012

TomPrince Said:

Given that, I think it makes sense to include the project as part of the repository, since (not using svn) it makes sense > to me that a svn url should be repository+branch.

But you don't use svn, so your point of while common and logical is not entirely applicable. Svn has a concept of a remotely accessible URL separate from the repository root. Look at the output of "svn info": The full url will include project and branch in an arbitrary order, while repository root is fixed. I'd like for the checkout step to use the repository in the source stamp, but to cover more than just my and jc2k's repos, we should keep project and repository separate to allow the all users to combine them appropriately. All repos are not set up repository+project+branch. repository+branch+project is also valid.

For the svn server, all subdirs past the repository root are arbitrary and have no inherent special meaning. The fact that we add that meaning confuses things quite a bit :-(

@tomprince

This comment has been minimized.

Show comment
Hide comment
@tomprince

tomprince Jun 6, 2012

Member

But the root of the repository is essentially a convention too, no? That is, it is entirely possible to treat an arbitrary subdir as the root repository.

Member

tomprince commented Jun 6, 2012

But the root of the repository is essentially a convention too, no? That is, it is entirely possible to treat an arbitrary subdir as the root repository.

@dwlocks

This comment has been minimized.

Show comment
Hide comment
@dwlocks

dwlocks Jun 6, 2012

Not quite. I think older versions of Subversion had a somewhat more, but not completely, arbitrary notion "repository root". However at least 1.5 and greater give the repository root as a fixed field in the svn info command. For example:

(weird error if you request info shallower than the true root)
15:17 $ svn info https://amanda.svn.sourceforge.net/svnroot
svn: Repository moved temporarily to 'http://amanda.svn.sourceforge.net/svnroot'; please relocate

15:17 $ svn info https://amanda.svn.sourceforge.net/svnroot/amanda
Path: amanda
URL: https://amanda.svn.sourceforge.net/svnroot/amanda
Repository Root: https://amanda.svn.sourceforge.net/svnroot/amanda

15:22 $ svn info https://amanda.svn.sourceforge.net/svnroot/amanda/amanda/trunk
Path: trunk
URL: https://amanda.svn.sourceforge.net/svnroot/amanda/amanda/trunk
Repository Root: https://amanda.svn.sourceforge.net/svnroot/amanda

dwlocks commented Jun 6, 2012

Not quite. I think older versions of Subversion had a somewhat more, but not completely, arbitrary notion "repository root". However at least 1.5 and greater give the repository root as a fixed field in the svn info command. For example:

(weird error if you request info shallower than the true root)
15:17 $ svn info https://amanda.svn.sourceforge.net/svnroot
svn: Repository moved temporarily to 'http://amanda.svn.sourceforge.net/svnroot'; please relocate

15:17 $ svn info https://amanda.svn.sourceforge.net/svnroot/amanda
Path: amanda
URL: https://amanda.svn.sourceforge.net/svnroot/amanda
Repository Root: https://amanda.svn.sourceforge.net/svnroot/amanda

15:22 $ svn info https://amanda.svn.sourceforge.net/svnroot/amanda/amanda/trunk
Path: trunk
URL: https://amanda.svn.sourceforge.net/svnroot/amanda/amanda/trunk
Repository Root: https://amanda.svn.sourceforge.net/svnroot/amanda

@dwlocks

This comment has been minimized.

Show comment
Hide comment
@dwlocks

dwlocks Jun 6, 2012

We can certainly decide to treat anything as the root, but that won't map to subversion's idea of the root. The "repository root" field (which we have used in the past, but may not now) doesn't change. That field used to be how we decided how much of the url to chop off to start splitting branch from project from filepath. If we don't do that anymore, then we're conceptually in the clear to redefine the repository root however we want. Otherwise, using svn's concept of repository root one place and buildbot's concept in another is just asking for confusion and bugs.

dwlocks commented Jun 6, 2012

We can certainly decide to treat anything as the root, but that won't map to subversion's idea of the root. The "repository root" field (which we have used in the past, but may not now) doesn't change. That field used to be how we decided how much of the url to chop off to start splitting branch from project from filepath. If we don't do that anymore, then we're conceptually in the clear to redefine the repository root however we want. Otherwise, using svn's concept of repository root one place and buildbot's concept in another is just asking for confusion and bugs.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Jun 6, 2012

Contributor

I'm not sure where we are reliant on svn's concept of repository root - i'm certainly not using that concept in my current setup. I configure my SVN step with a path somewhere deep within the repository and I configure my svn poller (using an earlier version of this code) so that the change has a matching repository and the branch is relative to that fake root. It works. The important thing is consistency.

Which means that this discussion isn't really a technical one - it's more about which strategy is best, which mental model is more appropriate, which strategy should we use by default. My changes don't presently restrict either approach, and arguably they wouldn't even if the project splitter modified the repository url.

So does this block my change?

Contributor

Jc2k commented Jun 6, 2012

I'm not sure where we are reliant on svn's concept of repository root - i'm certainly not using that concept in my current setup. I configure my SVN step with a path somewhere deep within the repository and I configure my svn poller (using an earlier version of this code) so that the change has a matching repository and the branch is relative to that fake root. It works. The important thing is consistency.

Which means that this discussion isn't really a technical one - it's more about which strategy is best, which mental model is more appropriate, which strategy should we use by default. My changes don't presently restrict either approach, and arguably they wouldn't even if the project splitter modified the repository url.

So does this block my change?

@dwlocks

This comment has been minimized.

Show comment
Hide comment
@dwlocks

dwlocks Jun 7, 2012

Yes, it's about mental models and default strategy, which is bikeshed territory. No, it doesn't block your change. So long as we don't box ourselves ( i.e. me ) to a particular model, all is good.

A few things:

  • We should make explicit that project should not be included in the SVNPoller's svnurl parameter if you care about the project attribute of changes. Perhaps I just missed that in the docs.
  • We should make explicit that a splitter can function as a change filter by returning None. (Maybe I also missed this in the docs).
  • We should rip out the inline comments at the top of Class SVNPoller that would be suddenly bogus and contradictory.

dwlocks commented Jun 7, 2012

Yes, it's about mental models and default strategy, which is bikeshed territory. No, it doesn't block your change. So long as we don't box ourselves ( i.e. me ) to a particular model, all is good.

A few things:

  • We should make explicit that project should not be included in the SVNPoller's svnurl parameter if you care about the project attribute of changes. Perhaps I just missed that in the docs.
  • We should make explicit that a splitter can function as a change filter by returning None. (Maybe I also missed this in the docs).
  • We should rip out the inline comments at the top of Class SVNPoller that would be suddenly bogus and contradictory.
@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jun 8, 2012

Member

This is looking fruitful!

@Jc2k, I agree that we've historically allowed the svn repository to be a pretty arbitrary prefix of the svn URL, regardless of where Subversion or Apache consider the repository to begin. I don't see a reason to change that here.

The 'project' attribute of changes is intended for use by change filters, not as a component to be included in calculating URLs during the build. It certainly can be used for that purpose (as can properties or just about anything else, using a renderable), but this should not be recommended.

So, I think that the suggested split function should return information such that join(repository, branch) points to the root of the codebase, along with appropriate filtering information (project, category). An "advanced" paragraph in the docs may suggest more complex use-cases.

Member

djmitche commented Jun 8, 2012

This is looking fruitful!

@Jc2k, I agree that we've historically allowed the svn repository to be a pretty arbitrary prefix of the svn URL, regardless of where Subversion or Apache consider the repository to begin. I don't see a reason to change that here.

The 'project' attribute of changes is intended for use by change filters, not as a component to be included in calculating URLs during the build. It certainly can be used for that purpose (as can properties or just about anything else, using a renderable), but this should not be recommended.

So, I think that the suggested split function should return information such that join(repository, branch) points to the root of the codebase, along with appropriate filtering information (project, category). An "advanced" paragraph in the docs may suggest more complex use-cases.

@tomprince

This comment has been minimized.

Show comment
Hide comment
@tomprince

tomprince Jun 18, 2012

Member

It occured to me, that with codebase support, people almost certainly will be wanting to set codebase instead of or in addition to project here.

Member

tomprince commented Jun 18, 2012

It occured to me, that with codebase support, people almost certainly will be wanting to set codebase instead of or in addition to project here.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jul 6, 2012

Member

I fear this devolved into some bike-shedding. It's also no longer cleanly applying against master.

Let's get to a least-common-denominator and get this merged, with notes to revisit any outstanding issues afterward.

Member

djmitche commented Jul 6, 2012

I fear this devolved into some bike-shedding. It's also no longer cleanly applying against master.

Let's get to a least-common-denominator and get this merged, with notes to revisit any outstanding issues afterward.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Jul 6, 2012

Contributor

Still on my radar, but held back by change priorities @ work and losing track of what was outstanding in the patch.

  • Fix merge
  • Add codebase parameter?
  • Rollback changes to the built in split functions as users may depend on the existing behaviour of returning a tuple
  • dwlocks suggested some doc tweaks. I'll try, but i'm terrible at docs.

Anything else?

Contributor

Jc2k commented Jul 6, 2012

Still on my radar, but held back by change priorities @ work and losing track of what was outstanding in the patch.

  • Fix merge
  • Add codebase parameter?
  • Rollback changes to the built in split functions as users may depend on the existing behaviour of returning a tuple
  • dwlocks suggested some doc tweaks. I'll try, but i'm terrible at docs.

Anything else?

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jul 6, 2012

Member

That looks like a good list to start with. Skimming the diff, I get about the same list:

  • fix merge conflicts
  • remove reference to SVNFile in split_file_project_branches (and maybe elsewhere)
  • fix default values in _transform_path
  • include the old way (tuples) in the docs, too, since that's what the existing functions do (compatibility sucks..)
  • allow split functions to return a codebase as well
  • don't try to do anything fancy with codebase or project - just pass them along to addChange (I think this part is done, just being clear)
  • add an entry to release-notes.rst.
Member

djmitche commented Jul 6, 2012

That looks like a good list to start with. Skimming the diff, I get about the same list:

  • fix merge conflicts
  • remove reference to SVNFile in split_file_project_branches (and maybe elsewhere)
  • fix default values in _transform_path
  • include the old way (tuples) in the docs, too, since that's what the existing functions do (compatibility sucks..)
  • allow split functions to return a codebase as well
  • don't try to do anything fancy with codebase or project - just pass them along to addChange (I think this part is done, just being clear)
  • add an entry to release-notes.rst.

@tomprince tomprince referenced this pull request Jul 18, 2012

Merged

Fix split_file_branches #472

@flaviojs

This comment has been minimized.

Show comment
Hide comment
@flaviojs

flaviojs Jul 19, 2012

Contributor

I read the Customizing SVNPoller section of the manual and apparently there are repositories that put projects inside the branches, so a split_file_branches_projects is in order.

PS. I would also rename split_file_project_branches to split_file_projects_branches (plural) since it works for multiple projects

Contributor

flaviojs commented Jul 19, 2012

I read the Customizing SVNPoller section of the manual and apparently there are repositories that put projects inside the branches, so a split_file_branches_projects is in order.

PS. I would also rename split_file_project_branches to split_file_projects_branches (plural) since it works for multiple projects

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jul 22, 2012

Member

The suggestion to rename is reasonable, but let's not add more work to this pull req. split_file_branches_projects can be added in another pull.

Member

djmitche commented Jul 22, 2012

The suggestion to rename is reasonable, but let's not add more work to this pull req. split_file_branches_projects can be added in another pull.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Aug 6, 2012

Contributor

Have rebased existing patch based on contents of buildbot-0.8.7

Contributor

Jc2k commented Aug 6, 2012

Have rebased existing patch based on contents of buildbot-0.8.7

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Aug 6, 2012

Contributor

Tried adding a codebase parameter but seems to be overriden by codebaseGenerator - is this expected/desired behaviour?

For me if i use the multi-codebase stuff the pollers will be where I know what to call the codebase.

Contributor

Jc2k commented Aug 6, 2012

Tried adding a codebase parameter but seems to be overriden by codebaseGenerator - is this expected/desired behaviour?

For me if i use the multi-codebase stuff the pollers will be where I know what to call the codebase.

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Aug 6, 2012

Contributor

Done. I think. Apart from the codebase thing, which i'd like to address before 0.8.7 if there is something to be done.

Contributor

Jc2k commented Aug 6, 2012

Done. I think. Apart from the codebase thing, which i'd like to address before 0.8.7 if there is something to be done.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Aug 6, 2012

Member

Ah, yes, codebaseGenerator was basically a way to not have every changesource determine codebases. The addChanges implementation in master.py should allow you to override that by passing a codebase explicitly. Does that not work?

Member

djmitche commented Aug 6, 2012

Ah, yes, codebaseGenerator was basically a way to not have every changesource determine codebases. The addChanges implementation in master.py should allow you to override that by passing a codebase explicitly. Does that not work?

@Jc2k

This comment has been minimized.

Show comment
Hide comment
@Jc2k

Jc2k Aug 7, 2012

Contributor

Fixed comment. Managed to get codebase working (had blamed addChange but was really my fault).

Also refactored how codebase/project/repo actually get set to mimimize the lines of code i actually change.

Contributor

Jc2k commented Aug 7, 2012

Fixed comment. Managed to get codebase working (had blamed addChange but was really my fault).

Also refactored how codebase/project/repo actually get set to mimimize the lines of code i actually change.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Aug 11, 2012

Member

Merged -- nice work, and thanks for your patience!

Member

djmitche commented Aug 11, 2012

Merged -- nice work, and thanks for your patience!

@djmitche djmitche closed this Aug 11, 2012

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