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

Create GitHub workflows #208

Merged
merged 3 commits into from
Oct 7, 2020
Merged

Conversation

DanielBadura
Copy link
Contributor

@DanielBadura DanielBadura commented Oct 5, 2020

This PR aims to add GitHub workflows in favor of travis and appveyor.
I think all the ci jobs are ported from travis beside of the nightly build check. I dont know if a solution exists atm but last time i checked this was not really possible in a "clean" way. See: actions/runner#2347

I also realized that we dont have any cs check atm. Maybe we should add them in the near future?

Also i think Action are not enabled yet here. See my test PR on my Repo for the output: DanielBadura#2
I mostly check at BetterReflection as a reference since I work more with GitLab-CI 🤖

I quite dont understand this part:

on:
    pull_request:
    push:
        branches:
            - "master"

Is on: pull_request: branches: [master] the default? Because i would think with this config workflow should only run if commits gets pushed into master. So i would expect this:

on:
    pull_request:
        branches:
            - "master"
    push:
        branches:
            - "master"

Because no job ran on the first commit i tried it in the psalm workflow but without success. So i figured that action are not enabled here and checked on my repo. There all workflows are running on the PR with or without on: pull_request: branches: [master]

@DanielBadura DanielBadura marked this pull request as ready for review October 5, 2020 21:47
@@ -0,0 +1,61 @@
# https://help.github.com/en/categories/automating-your-workflow-with-github-actions

name: "Static Analysis by PHPStan"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these perhaps extracted from ProxyManager or such? Looks very familiar :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From BetterReflection :P

Copy link
Collaborator

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

🚢 good work!

@Ocramius Ocramius added this to the 2.2.0 milestone Oct 7, 2020
@Ocramius Ocramius self-assigned this Oct 7, 2020
@Ocramius Ocramius added dependencies Pull requests that update a dependency file enhancement labels Oct 7, 2020
@Ocramius Ocramius merged commit 0f93b2b into maglnet:master Oct 7, 2020
@DanielBadura DanielBadura deleted the introduce-workflows branch October 7, 2020 17:11
@Ocramius Ocramius modified the milestones: 2.2.0, 3.0.0 Nov 8, 2020
@Ocramius Ocramius mentioned this pull request Nov 13, 2020
uses: "actions/cache@v2"
with:
path: |
~/.composer/cache
Copy link

Choose a reason for hiding this comment

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

Does Composer on Windows store the caches here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not, but given that we use a composer.lock, perhaps we can avoid this caching step overall?

Copy link

Choose a reason for hiding this comment

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

Well, there are "lowest" and "highest" deps in the matrix, which trigger composer update, which uses cache...

Yet there's a canonical way to handle this.

~/.composer/cache
vendor
key: "php-${{ matrix.php-version }}-${{ matrix.dependencies }}"
restore-keys: "php-${{ matrix.php-version }}-${{ matrix.dependencies }}"
Copy link

Choose a reason for hiding this comment

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

restore-keys should have php-${{ matrix.php-version }}- instead, the original key is assumed.

Copy link

Choose a reason for hiding this comment

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

And the key is lacking ${{ hashFiles('composer.*') }}

Copy link

Choose a reason for hiding this comment

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

I might open a PR with a comprehensive fix, if that's desirable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sanmai if you think the caching is botched and you have the time/will to do so, please do :)

Copy link

Choose a reason for hiding this comment

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

Sure thing: #227

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants