Skip to content

Conversation

shin-
Copy link
Contributor

@shin- shin- commented Sep 9, 2015

Needed for #756 - @aanand @dnephin Please take a look!

@shin- shin- self-assigned this Sep 9, 2015
@shin- shin- added this to the 1.4.0 milestone Sep 9, 2015
tests/Dockerfile Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple minor suggestions here:

  • best practices says to make these a single line, otherwise you can run into a situation where the update is cached without the available package you need to install
  • instead of easy_install pip why not add python-pip to the apt-get install line? If you want a newer version https://pip.pypa.io/en/latest/installing.html#install-pip suggests their install script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

best practices says to make these a single line, otherwise you can run into a situation where the update is cached without the available package you need to install

WIll update!

instead of easy_install pip why not add python-pip to the apt-get install line? If you want a newer version https://pip.pypa.io/en/latest/installing.html#install-pip suggests their install script.

"Surprisingly" (har har), python-pip is outdated/broken, so I'm using easy_install to get the latest version. I guess I could fetch their install script and use that instead, but that seems really unnecessary for what we're doing.

@shin-
Copy link
Contributor Author

shin- commented Sep 9, 2015

@aanand @dnephin PTAL!

@aanand
Copy link
Contributor

aanand commented Sep 9, 2015

I still don't like that those tests require access to the daemon's filesystem - we'll have to change them if we want to test against Swarm (which we should do). Still, that's arguably out of scope for this.

I think it could be squashed to fewer commits, then LGTM.

@shin-
Copy link
Contributor Author

shin- commented Sep 9, 2015

How else would you test volume binds though?

@shin- shin- force-pushed the 756-containerized-integration branch from 65c4cf0 to fa3082b Compare September 9, 2015 21:50
shin- added a commit that referenced this pull request Sep 9, 2015
@shin- shin- merged commit cfbc967 into master Sep 9, 2015
@shin- shin- deleted the 756-containerized-integration branch September 9, 2015 23:15
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.

3 participants