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

Switch CircleCI template to CircleCI 2.0. #115

Merged
merged 5 commits into from Mar 27, 2018

Conversation

@felicianotech
Contributor

felicianotech commented Jan 14, 2018

Fixes #52.

This is a first draft at converting the CircleCI template from using CircleCI 1.0 to 2.0

Notes:

  1. I haven't looked into optimizing the config yet. Just trying to do a basic conversion.
  2. The PHP Docker images are locked to PHP vX.Y, not a specific bug release. We can change that if the project owners decide they specifically want a full vX.Y.Z.
  3. I don't know Mustache well so this config may not be proper syntax for Mustache.
  4. I haven't run this config nor tested it yet. I plan to soon but others can join in as well. ;)
  5. The new config file should be located at ./.circleci/config.yml instead of ./circle.yml. How would I get that to work here?
@gitlost

Apologies for the delay in reviewing this @felicianotech , will review shortly after WP-CLI is released this Tuesday.

README.md Outdated
@@ -252,7 +252,7 @@ The following files are also included unless the `--skip-tests` is used:
default: travis
options:
- travis
- circle
- circleci

This comment has been minimized.

@gitlost

gitlost Jan 27, 2018

Contributor

This shouldn't be changed I think for BC.

This comment has been minimized.

@felicianotech

This comment has been minimized.

@gitlost

gitlost Jan 28, 2018

Contributor

Sorry, backward compatibility.

This comment has been minimized.

@felicianotech

felicianotech Jan 28, 2018

Contributor

Ah. Makes sense. I'll revert shortly.

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Jan 28, 2018

  • rebased on latest master
  • addressed comment about backwards compatibility
  • added some more CircleCI 2.0 config changes to the actual project outside of the new config itself.
@travislopes

This comment has been minimized.

travislopes commented Feb 12, 2018

Attempting to run this config errors out when trying to install the dependencies as it doesn't have permission to run aptitude. Run commands as sudo resolves that.

After fixing that, it errors out with mysqladmin: command not found not being found when running the install-wp-tests.sh script.

If I add mysql-server as a dependency, the command can then be found but cannot connect to the server:

mysqladmin: connect to server at '127.0.0.1' failed
error: 'Access denied for user 'ubuntu'@'127.0.0.1' (using password: NO)'
Exited with code 1

@gitlost gitlost modified the milestones: 1.1.2, 1.1.3 Feb 12, 2018

Oops, wrong PR.

@gitlost

This comment has been minimized.

Contributor

gitlost commented Feb 12, 2018

@travislopes thanks for trying it out, according to https://hub.docker.com/r/circleci/mysql/ the mysql user name is root.

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Feb 16, 2018

Added a comment addressing the latest comments here.

name: "Install Dependencies"
command: |
sudo apt-get update && sudo apt-get install subversion
sudo docker-php-ext-install mysqli

This comment has been minimized.

@travislopes

travislopes Feb 26, 2018

Installing the MySQLi extension seemed to do the trick. Tests ran perfectly for me with this new config.

This comment has been minimized.

@gitlost

gitlost Feb 26, 2018

Contributor

Thanks for the feedback @travislopes , stole that from #118. Not sure about the way it's installing mysql-client-5.7 (which is missing from circleci docker image), seems kludgey...

@gitlost

This comment has been minimized.

Contributor

gitlost commented Feb 26, 2018

@felicianotech took the liberty of pushing some commits to keep this moving.

Fixes up the tests, also uses sudo docker-php-ext-install mysqli from #118 to install the mysqli extension required by WP, also installs mysql-client-5.7 as mysqladmin missing from image and used in install-wp-test.sh - may not be doing this the best way.

An unrelated change that using MySQL 5.7 requires is making the Requires at least: in the readme.txt 4.5, instead of 4.4, due to this core fix https://core.trac.wordpress.org/ticket/34692 not being backported to 4.4 test suite.

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Feb 26, 2018

Awesome. Anything else needed from me?

@gitlost

This comment has been minimized.

Contributor

gitlost commented Feb 26, 2018

Cool, if you're happy with it @felicianotech then no, will merge! (edit: after getting review.)

@gitlost gitlost requested a review from wp-cli/committers Feb 26, 2018

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Feb 27, 2018

Nope, LGTM though there is conflicts now.

@felicianotech

This comment has been minimized.

Contributor

felicianotech commented Feb 27, 2018

Okay I merged in master to fix the conflicts.

@schlessera

schlessera approved these changes Mar 27, 2018 edited

Merging this for now (as my remaining doubts have only to do with my ignorance of CircleCI), so that it can gather real-world feedback.

@schlessera schlessera merged commit 8421115 into wp-cli:master Mar 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera changed the title from [WIP] Switch CircleCI template to CircleCI 2.0. to Switch CircleCI template to CircleCI 2.0. Mar 27, 2018

@schlessera

This comment has been minimized.

Member

schlessera commented Mar 27, 2018

Thanks for the pull-request (and the patience), @felicianotech !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment