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.

Sign up for GitHub

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

Jump to bottom

Docker/streamline image, upgrade old firefox with headless chrome #415

Merged
merged 122 commits into from
Nov 27, 2018
Merged

Docker/streamline image, upgrade old firefox with headless chrome #415

merged 122 commits into from
Nov 27, 2018

Conversation

s-pace
Copy link
Contributor

@s-pace s-pace commented Nov 22, 2018

This PR is here to streamline the docker image. It:

  • update the docker image with using the latest ubuntu image
  • update the nodejs used in the docker image, it uses nodejs 8
  • Enhance Dockerfile, use guidelines, remove noise
  • Use pipenv to elegantly install the python env
  • Remove the deprecated py3 parameter that is not used any more by travis
  • Change the docker:run command. It will by default use the remote steady image. A new parameter is introduced to use the local code instead
  • Rename the image and fixe the typo with scraper
  • Better log management by removing the redirection of stderr in stdout

cc #411

s-pace added 23 commits November 21, 2018 15:45
using latest ubuntu
f0f004a
updating nodejs to v8
2a19c67
using same current directory for every command
288ebdb
better test for docker:run
d4d684b
use clean up option from docker
5780e98
clearer use of os import
3f042ca
reformat and fix typo
ccd51ce
better syntax for apt-get-install
a353d07
wget quiet
4d6fb7e
use pipenv to install dependencies
b505503
removing unused arg, command and fix typo
c85b44e
removing unused arg, command and fix typo
1f16f12
use pipenv to run command
17602c1
fiwing missing wget
5595d29
fix missing ,
4cc443b
fixing typo, clean code, add module run
bffceb9
removing py3
5017612
removing unused option
5d99e7b
removing unused option
33b0796
fix missing ,
1ffdb70
use proper pipenv
5bbb3d6
reformat
3c6f25a
adding new parameter and change default behaviour of docker:build
31fc77d
pixelastic
pixelastic previously approved these changes Nov 22, 2018
View reviewed changes
Copy link
Contributor

@pixelastic pixelastic left a comment

Choose a reason for hiding this comment

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

Looks very good!

I'm just wondering about the renaming. Shouldn't we rename it to algolia/docsearch on Docker hub?

cli/src/commands/abstract_build_docker.py Outdated Show resolved Hide resolved
cli/src/commands/build_docker_scraper.py Outdated Show resolved Hide resolved
cli/src/commands/build_docker_scraper.py Outdated Show resolved Hide resolved
cli/src/commands/run_config_docker.py Outdated Show resolved Hide resolved
cli/src/commands/run_config_docker.py Show resolved Hide resolved
cli/src/commands/run_config_docker.py Show resolved Hide resolved
cli/src/commands/run_config_docker.py Outdated Show resolved Hide resolved
cli/src/commands/run_config_docker.py Outdated Show resolved Hide resolved
scraper/dev/docker/Dockerfile Show resolved Hide resolved
scraper/dev/docker/Dockerfile.base Show resolved Hide resolved
@pixelastic pixelastic mentioned this pull request Nov 22, 2018
Closed
@s-pace s-pace force-pushed the docker/streamline_image branch 11 times, most recently from bd1e8d0 to 6b8ce88 Compare November 26, 2018 13:03
pixelastic
pixelastic reviewed Nov 26, 2018
View reviewed changes
cli/src/commands/build_docker_scraper.py Outdated
"algolia/docsearch-scraper-test")

code = self.build_docker_file("scraper/dev/docker/Dockerfile.base",
"algolia/base-docsearch-scraper")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we have both images follow the same pattern in naming? Like docsearch-scraper-test and docsearch-scraper-base instead of base-docsearch-scraper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I will do it

cli/src/commands/run_config_docker.py Outdated Show resolved Hide resolved
scraper/dev/docker/Dockerfile.base Outdated
RUN apt-get update -y && apt-get install -yq \
curl \
wget \
sudo -yq \
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to add -yq to every line, once for the apt-get install command is enough

scraper/src/tests/config_loader/open_selenium_browser_test.py Outdated Show resolved Hide resolved
@s-pace
Copy link
Contributor Author

s-pace commented Nov 26, 2018

Review done @pixelastic
LMK if I can merge it

pixelastic
pixelastic approved these changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@pixelastic pixelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@s-pace s-pace merged commit a49d25c into master Nov 27, 2018
@s-pace s-pace deleted the docker/streamline_image branch November 27, 2018 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pixelastic pixelastic pixelastic approved these changes

@clemfromspace clemfromspace Awaiting requested review from clemfromspace

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@s-pace @pixelastic