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

Enhancement: Switch to using GitHub actions #714

Merged
merged 2 commits into from Nov 10, 2019

Conversation

localheinz
Copy link
Contributor

@localheinz localheinz commented Oct 15, 2019

This PR

  • switches from Travis CI to using GitHub actions
  • fixes issues reported via static analysis

@localheinz
Copy link
Contributor Author

Ha!

https://github.com/joindin/joindin-api/pull/714/checks?check_run_id=261434086#step:3:9

Problem 1

  • The requested PHP extension ext-intl * is missing from your system. Install or enable PHP's intl

uses: actions/checkout@master

- name: "Install dependencies with composer"
run: $(which composer) install
Copy link
Contributor

Choose a reason for hiding this comment

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

@localheinz would you try throwing a --ignore-platform-reqs here to see if this gets us past this issue of misisng ext/intl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about that, to be honest.

Let's see what we need ext/intl for!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running

$ composer remove ext-intl

followed by

$ composer require-checker

in #727 yields

> composer-require-checker --config-file=composer-require-checker.json
ComposerRequireChecker 2.1.20-250-g14887fc-dev
The following unknown symbols were found:
+----------------+--------------------+
| unknown symbol | guessed dependency |
+----------------+--------------------+
| Transliterator | ext-intl           |
+----------------+--------------------+
Script composer-require-checker --config-file=composer-require-checker.json handling the require-checker event returned with error code 1

so we actually need ext/intl.

Personally, I'd prefer not to run composer with the --ignore-platform-reqs options in CI.

Perhaps we can bug @nat with updating the images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svpernova09
Copy link
Contributor

Got the extensions installed, but still failing that it can't find the exention.

@localheinz localheinz force-pushed the feature/github-actions branch 7 times, most recently from 229e860 to 06d0ce1 Compare November 6, 2019 08:26
@localheinz localheinz force-pushed the feature/github-actions branch 7 times, most recently from 2f6eaf2 to 93a8311 Compare November 6, 2019 09:33
run: curl -s https://codecov.io/bash -o codecov

- name: "Send code coverage report to Codecov.io"
run: bash codecov -t ${{ secrets.CODECOV_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CODECOV_TOKEN, taken from https://codecov.io/gh/joindin/joindin-api/settings, needs to be provided as a secret in the repository settings (see https://help.github.com/en/github/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets#creating-encrypted-secrets).

Unfortunately, this has the limitation that the secrets aren't available for pull request builds (unless the author has write access).

Copy link
Contributor

Choose a reason for hiding this comment

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

CODECOV_TOKEN has been added

@localheinz localheinz force-pushed the feature/github-actions branch 3 times, most recently from 33f083f to 71beade Compare November 10, 2019 11:20
@localheinz
Copy link
Contributor Author

@svpernova09

Other than #784, is there anything holding up merging this?

@svpernova09
Copy link
Contributor

Other than #784, is there anything holding up merging this?

Been thinking back and forth on handling code style. As stated in the issue I'm in favor of Style-CI because it takes the burden off me-the-contributor-making-the-pr, and off me-the-code-reviewer reviewing the PR, and off the entire organization as something we can set and forget.

The Leadership Team had a discussion about this when Style-CI was proposed and the conclusion was to use Style-CI but not remove the tooling to run similar/hopefully the same style changes locally.

I also wouldn't want a code style issue to fail a CI/CD build. Yes the project has adopted a code style but it shouldn't be a sticking point to accept a PR and it shouldn't be a code review item because it's too silly not to automate. Especially when tooling makes it so easy.

I think what I'd prefer is to remove the Github action for "Coding Standards" and continue to allow Style-CI to automatically resolve CS issues.

@iansltx
Copy link
Contributor

iansltx commented Nov 10, 2019

Been meaning to take a look to see whether we can match StyleCI to PSR-12...and maybe not enforce any other opinions. Just haven't gotten around to it, but once that config is available it should be useful for other projects.

@localheinz
Copy link
Contributor Author

@svpernova09

This should be good to go then.

The requirement for Travis CI needs to be removed from https://github.com/joindin/joindin-api/settings/branches for master, then.

Copy link
Contributor

@svpernova09 svpernova09 left a comment

Choose a reason for hiding this comment

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

Thanks for all of the work here.

@svpernova09 svpernova09 merged commit 0588120 into joindin:master Nov 10, 2019
@localheinz localheinz deleted the feature/github-actions branch November 10, 2019 14:32
@localheinz
Copy link
Contributor Author

Thank you, @svpernova09!

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.

None yet

3 participants