Skip to content

Take codebase in consideration when doing a 'copy' or 'export' checkout.#1291

Merged
sa2ajj merged 3 commits intobuildbot:masterfrom
benallard:patch-3
Oct 28, 2014
Merged

Take codebase in consideration when doing a 'copy' or 'export' checkout.#1291
sa2ajj merged 3 commits intobuildbot:masterfrom
benallard:patch-3

Conversation

@benallard
Copy link
Copy Markdown
Contributor

Else, there is not much advantages of taking those mode, as the code will be fresh checked-out everytime ...

@benallard
Copy link
Copy Markdown
Contributor Author

I would have honestly expected the test to break ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'/' should be replaced by the correct platform dependant separator ... I just have no idea how to get it there ...

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.sep. And avoid using + for string manipulation

checkout = "{}{}{}".format(checkout, so.sep, self.codebase)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os sep will not work there, as the code is running on the master, and should be slave dependant ...

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, this is a good comment. what if the slave is windows?

The pattern for this is to use:

checkout_dir = self.build.slave.path_module.join(checkout_dir, self.codebase)

@sa2ajj
Copy link
Copy Markdown
Contributor

sa2ajj commented Oct 27, 2014

👍

Since we have this special code in place, I'd really like to see a test for it (and the fact that the test case did not fail before suggests there're not enough tests for this part).

@coveralls
Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 21bd446 on benallard:patch-3 into * on buildbot:master*.

@benallard
Copy link
Copy Markdown
Contributor Author

Actually, this code should be ... duplicated ... for each source step that implement a copy / export method ... Ouch !

Is there a way we could factorize it ?

@sa2ajj
Copy link
Copy Markdown
Contributor

sa2ajj commented Oct 27, 2014

Not very specific suggestion, but did you look at master/buildbot/steps/source/base.py, that's, how its name suggest, a base class of source checkout steps.

@benallard
Copy link
Copy Markdown
Contributor Author

I'm having troubles with the tests: exceptions.AttributeError: FakeBuild instance has no attribute 'slave' ...

@tardyp
Copy link
Copy Markdown
Member

tardyp commented Oct 27, 2014

Fakes are usually missing some features. You can just add the slave
Le 27 oct. 2014 22:19, "Benoît Allard" notifications@github.com a écrit :

I'm having troubles with the tests: exceptions.AttributeError: FakeBuild
instance has no attribute 'slave' ...


Reply to this email directly or view it on GitHub
#1291 (comment).

@benallard
Copy link
Copy Markdown
Contributor Author

The .slave was actually to much, I got that working, unfortunately, I was on the wrong branch ... Now trying to switch back to the right one ...

@benallard
Copy link
Copy Markdown
Contributor Author

Looks like it worked ! Now waiting for Travis ...

@benallard
Copy link
Copy Markdown
Contributor Author

Making it work for all the steps is another story ... We need to work on unifying them first. Please merge this one, and we can open another issue about the other steps ...

@coveralls
Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 5de61c8 on benallard:patch-3 into * on buildbot:master*.

@sa2ajj
Copy link
Copy Markdown
Contributor

sa2ajj commented Oct 28, 2014

@benallard could you please open a trac issue?

sa2ajj pushed a commit that referenced this pull request Oct 28, 2014
master/svn: Take codebase in consideration when doing a 'copy' or 'export' checkout.
@sa2ajj sa2ajj merged commit 65e60e3 into buildbot:master Oct 28, 2014
@benallard benallard deleted the patch-3 branch October 28, 2014 08:31
@benallard
Copy link
Copy Markdown
Contributor Author

@sa2ajj
Copy link
Copy Markdown
Contributor

sa2ajj commented Oct 28, 2014

Excellent, thank you :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants