Skip to content

Switch CircleCI template to CircleCI 2.0.#115

Merged
schlessera merged 5 commits intowp-cli:masterfrom
FelicianoTech:cci2
Mar 27, 2018
Merged

Switch CircleCI template to CircleCI 2.0.#115
schlessera merged 5 commits intowp-cli:masterfrom
FelicianoTech:cci2

Conversation

@FelicianoTech
Copy link
Contributor

@FelicianoTech 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?

@schlessera schlessera added command:scaffold-plugin-tests Related to 'scaffold plugin-tests' command command:scaffold-theme-tests Related to 'scaffold theme-tests' command labels Jan 15, 2018
gitlost
gitlost reviewed Jan 27, 2018
View reviewed changes
Copy link
Contributor

@gitlost gitlost left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

README.md Outdated
options:
- travis
- circle
- circleci
Copy link
Contributor

@gitlost gitlost Jan 27, 2018

Choose a reason for hiding this comment

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

This shouldn't be changed I think for BC.

Copy link
Contributor Author

@FelicianoTech FelicianoTech Jan 28, 2018

Choose a reason for hiding this comment

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

BC?

Copy link
Contributor

@gitlost gitlost Jan 28, 2018

Choose a reason for hiding this comment

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

Sorry, backward compatibility.

Copy link
Contributor Author

@FelicianoTech FelicianoTech Jan 28, 2018

Choose a reason for hiding this comment

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

Ah. Makes sense. I'll revert shortly.

@FelicianoTech
Copy link
Contributor Author

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
Copy link

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 previously approved these changes Feb 12, 2018
View reviewed changes
@gitlost gitlost modified the milestones: 1.1.2, 1.1.3 Feb 12, 2018
@gitlost gitlost dismissed their stale review February 12, 2018 14:59

Oops, wrong PR.

@gitlost
Copy link
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
Copy link
Contributor Author

FelicianoTech commented Feb 16, 2018

Added a comment addressing the latest comments here.

travislopes
travislopes approved these changes Feb 26, 2018
View reviewed changes
templates/plugin-circle.mustache
name: "Install Dependencies"
command: |
sudo apt-get update && sudo apt-get install subversion
sudo docker-php-ext-install mysqli
Copy link

@travislopes travislopes Feb 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@gitlost gitlost Feb 26, 2018

Choose a reason for hiding this comment

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

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
Copy link
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
Copy link
Contributor Author

FelicianoTech commented Feb 26, 2018

Awesome. Anything else needed from me?

@gitlost
Copy link
Contributor

gitlost commented Feb 26, 2018 •
edited
Loading

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

@gitlost gitlost requested a review from a team February 26, 2018 21:03
@gitlost gitlost mentioned this pull request Feb 26, 2018
Closed
@FelicianoTech
Copy link
Contributor Author

FelicianoTech commented Feb 27, 2018

Nope, LGTM though there is conflicts now.

@FelicianoTech
Copy link
Contributor Author

FelicianoTech commented Feb 27, 2018

Okay I merged in master to fix the conflicts.

schlessera
schlessera approved these changes Mar 27, 2018
View reviewed changes
Copy link
Member

@schlessera schlessera left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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
@schlessera schlessera changed the title [WIP] Switch CircleCI template to CircleCI 2.0. Switch CircleCI template to CircleCI 2.0. Mar 27, 2018
@schlessera
Copy link
Member

schlessera commented Mar 27, 2018

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

danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022
@schlessera
Merge pull request #115 from felicianotech/cci2
ce18087 
[WIP] Switch CircleCI template to CircleCI 2.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@schlessera schlessera schlessera approved these changes

+2 more reviewers

@travislopes travislopes travislopes approved these changes

@gitlost gitlost gitlost approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

command:scaffold-plugin-tests Related to 'scaffold plugin-tests' command command:scaffold-theme-tests Related to 'scaffold theme-tests' command

Projects

None yet

Milestone

1.1.3

Development

Successfully merging this pull request may close these issues.

4 participants

@FelicianoTech @travislopes @gitlost @schlessera