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

Add initial CI check #60

Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Feb 18, 2020

Related to #56, this adds an initial set of CI checks to be run via Travis.

To turn this check on:

  • Go to the Travis.com website.
  • Log in via GitHub ID.
  • Let it sync with GitHub to retrieve the repositories & turn the check on for this repo.

I've tested this PR - see here for a passing build: https://travis-ci.org/jrfnl/phpcs-security-audit/builds/651998291

Rulesets: Add XSD schema tags (PHPCS 3.2+/3.3.2+)

As of PHPCS 3.2.0, PHPCS includes an XSD schema for rulesets which defines what can be used in the XML ruleset file.

In PHPCS 3.3.0 a new array format for properties was introduced and as of PHPCS 3.3.2, the new array format is now also covered by this schema.

This commit adds the schema tags to the ruleset.xml file and the example rulesets.

Rulesets: tidy up

Tidy up the XML rulesets so they can pass validation against the XSD as well as for XML code style.

QA: add Parallel-Lint for easy syntax checking of the repo

Includes adding a composer lint convenience script to run the command.

The script as-is, is platform independent and will work on both Linux, Mac as well as Windows and respects the PHP version used by Composer.

Travis: add initial CI check

  • Validate the composer.json file.
  • Validate the XML ruleset files against the PHPCS XSD schema.
  • Verify the XML code style consistency of the ruleset files.
  • Run PHP linter over all PHP files on PHP 5.4, 7.4 and nightly (= PHP 8).

@jrfnl jrfnl mentioned this pull request Feb 18, 2020
9 tasks
@jmarcil
Copy link
Collaborator

jmarcil commented Feb 26, 2020

Great stuff.

But I'm afraid I have to ask this: what is the expected maintenance that a sole maintainer like me that can work on this project once every 6 months should expect from this integration?

Is it well recognized to never break and require no attention once it's setup? Or you've seen in the past that sometimes efforts needs to be done.

Right now I'm worry about adding complexity in places that provides no values for the users of this tool. This is just another thing for me to support and I'd prefer to put my time on things that the community has requested such as:

  • Getting better documentation
  • Doing a pass on getting better false positive reduction
  • Releasing v3 after some people test it

Let me know, worse case I think I'll icebox this PR and work on those things first, but if you say it's fire and forget then I'll just merge and move on.

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 27, 2020

Short answer: what I've done here is fire & forget, though the Travis integration does need to be turned on (one time action).

But I'm afraid I have to ask this: what is the expected maintenance that a sole maintainer like me that can work on this project once every 6 months should expect from this integration?

For the things in this PR, hardly anything, if anything at all.

The checks I've implemented here are more than anything to prevent incorrect code/inconsistencies being committed, so prevent issues from entering the repo and having to clean them up afterwards. (i.e. it should save you work, not give you work)

The only thing which may need an update every so often is the versions of the packages added in require-dev in the composer.json file, but as there is no committed composer.lock file (:heavy_check_mark:), that should only be needed on new major releases of the packages anyway.

Is it well recognized to never break and require no attention once it's setup? Or you've seen in the past that sometimes efforts needs to be done.

Well, tooling changes sometimes over time and new versions of things get released.

So small tweaks may be needed once in a while to keep up with the times - think: adding PHP 8 to the matrix once released -, but most of those are just maintenance and not something which would break the build if not done.

Right now I'm worry about adding complexity in places that provides no values for the users of this tool.

Except that it does add value. Having CI checks in place, prevent issues from entering the repo and getting to end-users.
For example: with the checks in place now, an accidental parse error in a sniff will cause the build to fail, instead of it being merged into the repo silently and nobody noticing it until a bug report comes in.

Let me know, worse case I think I'll icebox this PR and work on those things first, but if you say it's fire and forget then I'll just merge and move on.

I'd suggest merge & move on.

CI checks like this are intended to take work away from you by automating things you'd otherwise would need to check manually.

So, in a next step, we could add automated testing, like discussed elsewhere. That way for any PR, you know that it won't negatively affect existing checks without having to do a lot of testing for that yourself.

Same, having automated tests in place should also help with the "reduce false positives" project.

This is just a first set up and it can be expanded with additional checks as described in #56 to make the project easier to maintain in the long run.

As of PHPCS 3.2.0, PHPCS includes an XSD schema for rulesets which defines what can be used in the XML ruleset file.

In PHPCS 3.3.0 a new array format for properties was introduced and as of PHPCS 3.3.2, the new array format is now also covered by this schema.

This commit adds the schema tags to the `ruleset.xml` file and the example rulesets.
@jrfnl jrfnl force-pushed the feature/add-initial-ci-check branch from 78abe44 to 7af47a2 Compare February 27, 2020 19:45
Tidy up the XML rulesets so they can pass validation against the XSD as well as for XML code style.
Includes adding a `composer lint` convenience script to run the command.

The script as-is, is platform independent and will work on both Linux, Mac as well as Windows and respects the PHP version used by Composer.
* Validate the `composer.json` file.
* Validate the XML ruleset files against the PHPCS XSD schema.
* Verify the XML xode style consistency of the ruleset files.
* Run PHP linter over all PHP files.
@jrfnl jrfnl force-pushed the feature/add-initial-ci-check branch from 7af47a2 to 7456379 Compare February 27, 2020 19:49
@jmarcil jmarcil merged commit 1b8a61d into FloeDesignTechnologies:master Feb 28, 2020
@jmarcil
Copy link
Collaborator

jmarcil commented Feb 28, 2020

Sold!

I'd like to point out that the website is not Travis.com like mentioned above but rather travis-ci.org.

Also, I'm not happy about the fact that by default I have to authorize travis to all my repos to just log in.

@jrfnl jrfnl deleted the feature/add-initial-ci-check branch March 4, 2020 04:40
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 4, 2020

I'd like to point out that the website is not Travis.com like mentioned above but rather travis-ci.org.

My bad. Quick replies while traveling and detail precision don't always go well together ;-)

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.

2 participants