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

PHPLIB-610: Psalm Integration #973

Merged
merged 14 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/workflows/coding-standards.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@ on:
branches:
- "v*.*"
- "master"
- "feature/*"
Copy link

Choose a reason for hiding this comment

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

Do you really want to run the CI on PR targetting a feature branch ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, if such a PR were to exist.

push:
branches:
- "v*.*"
- "master"
- "feature/*"
Copy link

Choose a reason for hiding this comment

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

As you use such branches to open PRs, this will trigger 2 CI builds for each push (one for the push to the branch and one for the update to the PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

I push my branches to GitHub usually long before opening a pull request. This allows me to test a wider range of environments than I cover locally (before pushing) and ensuring things are stable and done before opening a pull request.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, we have only ever used feature branches a handful of times when a large PR was ready but we weren't completely done with the JIRA epic (e.g. SDAM Monitoring in PHPC). In that case, we opened PRs against a feature branch from arbitrary branches on our forks and the feature branch was a waiting area before we could merge the functionality down the master.

I don't expect we will push directly to feature branches during development, so this shouldn't invite more CI builds than our typical workflow of PRs against master and release branches. @alcaeus: please correct me if I'm mistaken.

Copy link

Choose a reason for hiding this comment

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

@jmikola the current PR is opened from a feature branch...

Copy link
Member Author

Choose a reason for hiding this comment

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

To summarise what we're currently testing and why it's useful:

PRs to any versioned branches (v*.*), the master branch and any feature branch (feature/*) should be tested, as we're using them to ensure we're only merging good code. The latter will not occur very often, but it could if we ever decide to build a larger feature in multiple PRs but don't want any logic in master before it's completely done.

Pushes to versioned branches and the master branch are important in the main repo. Pushes to feature branches will not occur very often here, however they do occur in forks (where we do our development work before creating a PR to the main repo). For quick PRs where we don't want to know if the checks are stable we can omit the feature/ prefix to skip CI before making a PR.

Enabling builds on push is also done for the benefit of outside contributors: this allows them to see CI results before creating a pull request even when they don't have a good test environment set up locally.

Copy link
Member

Choose a reason for hiding this comment

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

@jmikola the current PR is opened from a feature branch...

Totally missed that. @alcaeus and I talked about this today and I noted our workflows differ. I only recalled using feature branches on the mongodb repo when we wanted to merge down larger PRs without putting them into master. Andreas explained this is more for his fork, which inherits the workflow configuration.

He was last looking at whether some repo config options were available to conserve CI resources, so I'll defer to him on this.


jobs:
coding-standards:
name: "Coding Standards"
phpcs:
name: "phpcs"
runs-on: "ubuntu-20.04"

strategy:
Expand Down
68 changes: 68 additions & 0 deletions .github/workflows/static-analysis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
name: "Static Analysis"

on:
pull_request:
branches:
- "v*.*"
- "master"
- "feature/*"
push:
branches:
- "v*.*"
- "master"
- "feature/*"

jobs:
psalm:
name: "Psalm"
runs-on: "ubuntu-20.04"

strategy:
matrix:
php-version:
- "7.4"
driver-version:
- "mongodb/mongo-php-driver@master"

steps:
- name: "Checkout"
uses: "actions/checkout@v2"

- name: Setup cache environment
id: extcache
uses: shivammathur/cache-extensions@v1
with:
php-version: ${{ matrix.php-version }}
extensions: "mongodb-${{ matrix.driver-version }}"
key: "extcache-v1"

- name: Cache extensions
uses: actions/cache@v2
with:
path: ${{ steps.extcache.outputs.dir }}
key: ${{ steps.extcache.outputs.key }}
restore-keys: ${{ steps.extcache.outputs.key }}

- name: "Install PHP"
uses: "shivammathur/setup-php@v2"
with:
coverage: "none"
extensions: "mongodb-${{ matrix.driver-version }}"
php-version: "${{ matrix.php-version }}"
tools: "cs2pr"

- name: "Show driver information"
run: "php --ri mongodb"

- name: "Cache dependencies installed with Composer"
uses: "actions/cache@v2"
with:
path: "~/.composer/cache"
key: "php-${{ matrix.php-version }}-composer-locked-${{ hashFiles('composer.lock') }}"
restore-keys: "php-${{ matrix.php-version }}-composer-locked-"

- name: "Install dependencies with Composer"
run: "composer install --no-interaction --no-progress --no-suggest"

- name: "Run Psalm"
run: "vendor/bin/psalm --show-info=false --stats --output-format=github --threads=$(nproc)"
26 changes: 26 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,32 @@ To automatically fix all fixable errors, use the `phpcbf` binary:
$ vendor/bin/phpcbf
```

## Running static analysis

The library uses [psalm](https://psalm.dev) to run static analysis on the code
and ensure an additional level of type safety. New code is expected to adhere
to level 1, with a baseline covering existing issues. To run static analysis
checks, run the `psalm` binary:

```
$ vendor/bin/psalm
```

To remove fixed errors from the baseline, you can use the `update-baseline`
command-line argument:

```
$ vendor/bin/psalm --update-baseline
```

Note that this will not add new errors to the baseline. New errors should be
fixed instead of being added to the technical debt, but in case this isn't
possible it can be added to the baseline using `set-baseline`:

```
$ vendor/bin/psalm --set-baseline=psalm-baseline.xml
```

## Documentation

Documentation for the library lives in the `docs/` directory and is built with
Expand Down
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
"require-dev": {
"squizlabs/php_codesniffer": "^3.6",
"doctrine/coding-standard": "^9.0",
"symfony/phpunit-bridge": "^5.2"
"symfony/phpunit-bridge": "^5.2",
"vimeo/psalm": "^4.x-dev"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to include changes from vimeo/psalm#8432 which haven't been released yet. In the future we may consider shipping a plugin so we can control releases ourselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed PHPLIB-984 to track updating this.

Copy link

Choose a reason for hiding this comment

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

is it expected that it does not allow using stable releases ?

Copy link
Member Author

Choose a reason for hiding this comment

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

See my original comment: #973 (comment)

We're currently relying on unreleased fixes to the MongoDB call maps.

},
"autoload": {
"psr-4": { "MongoDB\\": "src/" },
Expand Down