Skip to content
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

[ADD] runbot_buildout #154

Merged
merged 3 commits into from
Sep 18, 2018
Merged

[ADD] runbot_buildout #154

merged 3 commits into from
Sep 18, 2018

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Apr 26, 2018

No description provided.

@hbrunn hbrunn added this to the 11.0 milestone Apr 26, 2018
if os.path.exists(self._path(odoo_part, 'odoo')):
return self._path(odoo_part, 'odoo', *l, **kw)
return self._path(odoo_part, 'openerp', *l, **kw)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

else-return is not needed if you if section uses a return
You can use just return ... without else

Copy link
Contributor

Choose a reason for hiding this comment

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

What about use return early?

if not  self.repo_id.uses_buildout:
    return ...
# My code for uses_buildout without nested `if` and without `else` reduce cyclomatic complex

lock_path, log_path,
)
else:
return super(RunbotBuild, self)._job_00_init(
Copy link
Contributor

Choose a reason for hiding this comment

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

if I have build.repo_id.uses_buildout=True but don't have build.branch_id.buildout_version should continue the normal runbot process?

@api.multi
def _spawn_buildout(self, cmd, lock_path, log_path):
self.ensure_one()
out = open(log_path, "w")
Copy link
Contributor

Choose a reason for hiding this comment

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

When is this file closed?

Copy link
Member Author

Choose a reason for hiding this comment

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

by now a few lines below, thanks for this one

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, What about use with open(log_path, "w") in order to ensure that this file is closed if there is an error in the middle.

})
self._github_status()
return False
buildout_file = tempfile.NamedTemporaryFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

What about use with?

with tempfile... as buildout_file:
    buildout_file.write(adapted_buildout)

'branch_id': branch.id,
'name': '42',
})
self.assertIn('parts/', build._server())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is possible add a real tests running all jobs using a repo of example real?

@moylop260
Copy link
Contributor

Oh sorry I didn't the WIP label

@hbrunn
Copy link
Member Author

hbrunn commented Apr 26, 2018

thanks for your input anyways, some of your points I didn't think about

@hbrunn hbrunn mentioned this pull request May 3, 2018
@hbrunn hbrunn force-pushed the 11.0-runbot_buildout branch 3 times, most recently from ac4c3c7 to 4e71038 Compare May 31, 2018 16:56
@hbrunn
Copy link
Member Author

hbrunn commented May 31, 2018

@moylop260 this is reviewable by now. Seems like there was an incompatible change in runbot: https://travis-ci.org/OCA/runbot-addons/jobs/386271662#L2709

@moylop260
Copy link
Contributor

@hbrunn
FYI I have fixed from
#160

@hbrunn hbrunn force-pushed the 11.0-runbot_buildout branch 3 times, most recently from cbeb45c to 2ad3b1f Compare June 5, 2018 16:57
) as buildout_file:
buildout_file.write(bytes(
'[buildout]\n'
'extensions = buildout.wheel\n'

Choose a reason for hiding this comment

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

Probably we then also have to change this?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, and I'm going to ruhtlessly steal your snippet to use newer setuptools

@hbrunn hbrunn force-pushed the 11.0-runbot_buildout branch 4 times, most recently from 80019a9 to 2782c1d Compare June 7, 2018 14:11
@emagdalenaC2i emagdalenaC2i mentioned this pull request Jun 21, 2018
6 tasks
@hbrunn hbrunn force-pushed the 11.0-runbot_buildout branch 3 times, most recently from 1f1abe1 to b54447b Compare June 28, 2018 06:22
.travis.yml Outdated
@@ -1,7 +1,7 @@
language: python

python:
- "3.5"
- "3.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why python 3.6?
I mean, odoo uses by default 3.5

Copy link
Member Author

Choose a reason for hiding this comment

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

i have no idea. Must have been a leftover from me working frantically to get the tests running, there were very weird errors

.travis.yml Outdated
# only install extra dependencies when we're not linting
- pip install zc.recipe.egg --no-cache-dir
# the latest package on pip doesn't speak python3
- pip install https://github.com/anybox/anybox.recipe.odoo/tarball/master/anybox.recipe.odoo-1.9.3.dev0.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not good use requirements.txt for this case, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to pin this to a git+https url, but that didn't work the first couple of tries and then I figured I'll just wait it out until this version is released

@api.multi
def _spawn_buildout(self, cmd, lock_path, log_path):
self.ensure_one()
out = open(log_path, "w")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, What about use with open(log_path, "w") in order to ensure that this file is closed if there is an error in the middle.

Copy link
Contributor

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

👍

@moylop260
Copy link
Contributor

Do you have planned squash the commits or you prefer that I use merge&squash github button?

@hbrunn
Copy link
Member Author

hbrunn commented Jul 5, 2018

I squashed them, thanks!

runbot_buildout/models/runbot_build.py Outdated Show resolved Hide resolved
runbot_buildout/tests/test_runbot_buildout.py Outdated Show resolved Hide resolved
runbot_buildout/tests/test_runbot_buildout.py Outdated Show resolved Hide resolved
@hbrunn hbrunn force-pushed the 11.0-runbot_buildout branch 3 times, most recently from 0570e17 to 1982c66 Compare September 18, 2018 07:43
@hbrunn
Copy link
Member Author

hbrunn commented Sep 18, 2018

@daramousk @thomaspaulb done

.travis.yml Outdated

before_install:
# Fix https://github.com/travis-ci/travis-ci/issues/8982#issuecomment-354357640
- python3.5 -c "import fcntl; fcntl.fcntl(1, fcntl.F_SETFL, 0)"

install:
- pip install zc.recipe.egg --no-cache-dir
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try using requirements.txt file?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll try now

@moylop260 moylop260 merged commit f88bc3e into OCA:11.0 Sep 18, 2018
NL66278 pushed a commit to NL66278/runbot-addons that referenced this pull request Jan 23, 2019
gitlab uses json but runbot uses http since that github supports both
(json and http)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants