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

Use circleci/python for dev image and install RDB servers. #1495

Merged

Conversation

toshihikoyanase
Copy link
Member

@toshihikoyanase toshihikoyanase commented Jul 9, 2020

Motivation

The current docker images do not contain CircleCI related files, so we cannot use it to execute CI jobs locally.
In addition, it lacks DB servers and we need to install them to test with such servers.

Description of the changes

This PR will apply the following changes:

  • Change base image from python to circleci/python.
  • Add DBs, i.e., default-mysql-server, default-mysql-client, postgresql, and postgresql-client
  • Python DB bindings

@toshihikoyanase toshihikoyanase marked this pull request as ready for review July 9, 2020 02:09
@toshihikoyanase toshihikoyanase added the CI Continuous integration. label Jul 9, 2020
Copy link
Contributor

@crcrpar crcrpar left a comment

Choose a reason for hiding this comment

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

Changes look solid.
Maybe I misunderstand the object, though, could you tell me how to use the images on DockerHub for local CircleCI execution?

@toshihikoyanase
Copy link
Member Author

@crcrpar Thank you for your review.

could you tell me how to use the images on DockerHub for local CircleCI execution?

Sorry for the lack of description.
I'll simply edit the .circleci/config.yml to replace the docker image. For example, we can use the DockerHub image to skip the installation process as follows:

  checks:
    docker:
      - image: toshihikoyanase/optuna:py3.7-dev
      # - image: circleci/python:3.7
    steps:
      - checkout

      - run: &checkout-merge-master
          name: checkout-merge-master
          command: |
            set -ex
            if [[ -n "${CIRCLE_PR_NUMBER}" ]]; then
                FETCH_REFS="${FETCH_REFS} +refs/pull/${CIRCLE_PR_NUMBER}/merge:pr/${CIRCLE_PR_NUMBER}/merge"
                git fetch -u origin ${FETCH_REFS}
                git checkout "pr/${CIRCLE_PR_NUMBER}/merge"
            fi

      # - run:
      #     name: install
      #     command: |
      #       python -m venv venv || virtualenv venv
      #       . venv/bin/activate
      #       pip install -U pip
      #       pip install --progress-bar off .[checking]

      - run:
          name: black
          command: |
            # . venv/bin/activate
            black . --line-length 99 --check --exclude="docs"

At first, I thought we could also use it in daily CI to skip the installation, but we can't actually. It will not work if developers change the dependencies in their PRs.

Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

LGTM.

@HideakiImamura HideakiImamura merged commit 79927f3 into optuna:master Jul 13, 2020
@toshihikoyanase toshihikoyanase deleted the circleci-image-for-dev branch July 13, 2020 06:12
@HideakiImamura HideakiImamura added this to the v2.0.0 milestone Jul 13, 2020
@HideakiImamura HideakiImamura self-assigned this Jul 13, 2020
HideakiImamura added a commit that referenced this pull request Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants