-
Notifications
You must be signed in to change notification settings - Fork 33
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
Gentoo Integration Tests #115
Conversation
@tfoote This is the current testing framework. Do you think this will suffice?
|
That looks reasonable. Where |
Newly fixed output. |
Yes.
That's how I'm seeing it, at least. This tech has already proven itself invaluable. I think there should be a follow-up PR with the actual test mechanism built into it. My current thought is that the list is saved in the branch we are testing on the overlay, Something like this:
As an added advantage, this may be run locally.
|
superflore/docker.py
Outdated
def build(self): | ||
if not self.dockerfile_directory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the only place that the dockerfile is used so maybe it should be an argument for this method instead of a constructor argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's a good point.
superflore/docker.py
Outdated
@@ -20,9 +20,10 @@ | |||
|
|||
|
|||
class Docker(object): | |||
def __init__(self, dockerfile, name): | |||
def __init__(self, dockerfile=None, name=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see self.name
being used anywhere is it necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I missed that. It certainly isn't used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small comments. And it would be good to have some simple tests for pull and build.
superflore/docker.py
Outdated
|
||
def build(self, dockerfile): | ||
dockerfile_directory = os.path.dirname(dockerfile) | ||
if not dockerfile_directory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check doesn't seem right at the moment. It should somehow verify that the Dockefile is a valid dockerfile not that the directory exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I meant that you should not just check that it's a directory but check that the Dockerfile itself exists. (aka check the file that is passed exists and then lookup the directory to pass on.) And even better would be to see if the Dockerfile parses/validates but that's extra.
tests/test_docker.py
Outdated
@@ -18,14 +18,12 @@ | |||
|
|||
class TestDocker(unittest.TestCase): | |||
def get_image(self): | |||
docker_file = resource_filename('repoman_docker', 'Dockerfile') | |||
dock = Docker(docker_file, 'gentoo_repoman') | |||
dock = Docker() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole function seems outdated now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was keeping it for legacy reasons. ;)
Removed.
1e16f41
to
f3d32b3
Compare
@tfoote new coverage report.
EDIT: Ok, I'm actually not sure why it's not counting most of the |
Figured it out. If you have a folder named |
The issue here seems to be an authentication error, since one needs to login before they can pull. As such, I'm adding a |
…obnoxious to do without user interaciton. There's a reason Gentoo doesn't have an automated installer yet.
…o mask it for now.
as relax the requiered dockerfile directory arg in the __init__ function.
…mpt to build the requested target.
ubuntu is an official repository so it doesn't have an organization/username element to the image you should use |
…up trying to interpret it.
Yep, figured it was just me. Thanks! Working fine now. |
@tfoote Travis is really slow atm, but I suspect this one will pass. |
for pkg in sorted(self.package_list.keys()): | ||
self.container.add_bash_command('emaint sync -r ros-overlay') | ||
self.container.add_bash_command('emerge %s' % pkg) | ||
self.conainter.login() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling conainter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. That line didn't need to be there in the first place, actually.
superflore/docker.py
Outdated
def login(self): | ||
# TODO(allenh1): add OAuth here, and fall back on user input | ||
# if the OAuth doesn't exist (however one finds that). | ||
user = getpass('Docker user:') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this fallback if in non-interactive shells?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. No it does not. I'll use the environment variables I set on the CI if they are available, then do a os.isatty(0)
to throw an exception if they aren't on a tty.
@@ -0,0 +1,52 @@ | |||
# Copyright 2018 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file is not used yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at the moment. My plan was to use it in a follow up PR (since we're many commits in at this point).
But I suppose I can start the superflore-check-ebuilds
in this PR.
@tfoote Ok, added the |
…er to use extended permissions (such as SYS_PTRACE, which is needed for building glibc).
* superflore.docker.py: Add pull function, as well as relax the requiered dockerfile directory arg in the __init__ function. * Add a GentooBuilder class that will spawn a docker container and attempt to build the requested target. * Add a function to clear the bash commands. * Add run function to build_base.py. This function tests each of the desired packages in the spawned docker container. * Added output to run function. * Refactor docker class to reduce parameters. * Remove outdated code and fix build condition. * Remove unused function. * Add test docker build. * Add test pull and test run. * Fix typo in setup.py. * Add a login function to the docker class * Build the test Dockerfile to test the run function. * Add tag flag to pull function. * Add support for environment variable login and detection of a tty for login. * Add a superflore-check-ebuilds command. * Add support for the yaml file holding the packages to test. * Document the new script. * Add privileged flag to the run function to allow docker to allow docker to use extended permissions (such as SYS_PTRACE, which is needed for building glibc).
Opening for visibility. This is very, very rough.
Step one here (to me, at least) is to get docker to update a Gentoo image to be ready to emerge
ros-lunar/ros_base
, which is currently hard-coded into the script.Not sure if this is working, as we are emerging ~300 packages after the
USE
flag changes, so I'll report back whenever that finishes (hopefully no webkit).