Migrate travis github actions #571

Shaky212 · 2022-12-21T14:02:53Z

To gain some speed during the ci/cd proces we decided to migrate travis to the github actions.

The biggest task was putting the scripting into separate shell scripts. The reason for this is that we can access the scripts within composer. Some tasks are still allowed to fail in the test integration workflow, such as the copy paste detector and cypress. It is intended that these will be improved at a later stage.

MKodde

Yeehaw, great work partner! 🤠

See some possible improvements and some questions about your proposal I'd like to discuss before merging this PR.

MKodde · 2022-12-22T12:03:10Z

.github/workflows/main.yml

@@ -11,7 +11,7 @@ on:
 jobs:
 build:

- runs-on: ubuntu-latest
+ runs-on: ubuntu-20.04


Is this intentional?

Yes, due to the cgroups v1 issue see the fix of @quartje at 3794e8c

MKodde · 2022-12-22T12:03:50Z

.github/workflows/test-integration.yml

@@ -9,64 +9,89 @@ on:
 jobs:
 build:

- runs-on: ubuntu-latest
+ runs-on: ubuntu-20.04


Why run the tests on the old Ubuntu LTS?

See previous comment

MKodde · 2022-12-22T12:06:42Z

.github/workflows/test-integration.yml

+ - name: Check out the repo
+ uses: actions/checkout@v2
+
+ - name: Start the docker images for Cypress testing


Nitpick: I think you can broaden the name, as we no longer solely run Cypress tests.

Correctamente

MKodde · 2022-12-22T12:07:44Z

.github/workflows/test-integration.yml

+ run: |
+ ${DOCKER_COMPOSE_PHP_FPM} /usr/local/bin/composer install -n --prefer-dist -o --ignore-platform-reqs
+ env:
+ SYMFONY_ENV: dev


Can we run the application in test or even prod? That might be better than dev

MKodde · 2022-12-22T12:09:12Z

.github/workflows/test-integration.yml

+ - name: Run Copy Paste Detector
+ run: |
+ ${DOCKER_COMPOSE_PHP_FPM} sh -c 'composer jscpd'
+ continue-on-error: true
+
+ - name: Run CI tests
+ run: |
+ ${DOCKER_COMPOSE_PHP_FPM} sh -c 'composer check'


I'd say: make the jscpd part of the composer check (Run CI tests)

I have deliberately chosen to keep it out of the CI tests for now, otherwise it breaks the CI tests. Or is there a way to continue anyway?

MKodde · 2022-12-22T12:12:12Z

ci/qa/jscpd

+# - allowing for 30 lines of duplicate code
+# - and setting a total copy paste limit to 1% project wide
+# More info about jscpd usage: https://github.com/kucherenko/jscpd/tree/master/packages/jscpd#usage
+./node_modules/.bin/jscpd src -l 30 -t 1


Did you not end up adding the excluded folders? If this command does not give too many false positives, I'ts fine with me. I do think we need to both check the JS and PHP code though. And the JS code is not in the src folder IIRC.

MKodde · 2022-12-22T12:14:33Z

ci/qa/translations

+#!/usr/bin/env sh
+
+cd $(dirname $0)/../../
+
+./bin/console lexik:translations:import
+./bin/console lexik:translations:import DashboardBundle
+./bin/console lexik:translations:import DashboardSamlBundle


I think this file should move to the /bin directory as this is not a qa related script. It is used during development to extract translations from the application into the lexik translation database.

Correctamente

MKodde · 2022-12-22T12:15:36Z

composer.json

+ "@cvalidate",
+ "@attrsc",
+ "@attrdc",
+ "@droptestdb",


Simply run @test-db here?

Absolutely!

MKodde · 2022-12-22T12:16:48Z

composer.json

+ "phpcov": "./ci/qa/phpcov",
+ "test": "./ci/qa/phpunit",
+ "cypress": "./ci/qa/cypress",
+ "yarn-audit": "./ci/qa/yarn-audit",


Suggestion: rename the yarn-audit script (and Composer script) to security-check (or another cool name) and run both yarn audit and compser audit in one go.

Yes.. simple security might be a good candidate

MKodde · 2022-12-22T12:17:42Z

docker/Dockerfiledev

@@ -9,17 +9,13 @@ CMD ["/usr/local/sbin/start.sh"]

 FROM ghcr.io/openconext/openconext-containers/openconext-phpfpm-dev:latest AS spdphpfpm
 WORKDIR /var/www/html/
-COPY --from=composer:1 /usr/bin/composer /usr/local/bin/composer
+COPY --from=composer:2.2.9 /usr/bin/composer /usr/local/bin/composer


Suggestion: remove the version pin. The latest composer will be pulled. This ensures we stay up to date with the latest composer features.

The phpcpd gave problems and I tried without versioning and also with :2 and finally it went on with 2.2.9., but finally ended up due to xdebug extension problems. Since phpcpd is no longer used we can use the :latest version I guess.

MKodde · 2022-12-22T12:23:24Z

.github/workflows/test-integration.yml

+
+ - name: Run Cypress tests
+ run: |
+ ${DOCKER_COMPOSE_PHP_FPM} sh -c 'composer cypress'


Shouldn't this run on the cypress container? It now errors without running a Cypress test

Ah, that may be the case.

MKodde · 2022-12-22T12:24:08Z

.github/workflows/test-integration.yml

+ - name: Run Audit
+ run: |
+ ${DOCKER_COMPOSE_PHP_FPM} sh -c 'composer yarn-audit'
+ continue-on-error: true


I'm skeptical as to allow a failing yarn/composer audit.. I thik you should remove this continue-on-error..

Yes..., but a lot of critical and high risk vulnerabilities found, that need to be fixed in the end, I guess (during upgrade SF?)

MKodde

As long as you re-enable the security tests to not be an allowed failure, this is fine with me!

Nice work to migrate to Github Actions :

* Upgrade Symfony 5 A mighty PR where multiple disciplines entailing the Symfony upgrade come together. Among them: #562 Upgrade to Symfony 4 (in preparation to this SF5 upgrade) #571 Move to Github actions for test integrations At first these tasks where created separate. But to keep things more tight, I chose to merge/rebase them into this branch. Beside an upgrade to this version a major update of the security has been done. Most of the security part is handled within the stepup saml bundle and SPD is a consumer of this implementation. see: https://www.pivotaltracker.com/story/show/173217517 Co-authored-by: Michiel Kodde <mkodde@ibuildings.nl> Co-authored-by: Bart Geesink <bart.geesink@surf.nl>

Shaky212 force-pushed the feature/migrate-travis-to-github-actions branch 6 times, most recently from 1008645 to 53ae991 Compare December 22, 2022 10:20

Migrate travis ci/cd to github actions

32f5868

Shaky212 force-pushed the feature/migrate-travis-to-github-actions branch from 53ae991 to 32f5868 Compare December 22, 2022 10:30

MKodde reviewed Dec 22, 2022

View reviewed changes

Shaky212 force-pushed the feature/migrate-travis-to-github-actions branch 4 times, most recently from 67812b9 to fad6d99 Compare December 23, 2022 07:57

Refine and rename scripts based on review

16e0ebb

Shaky212 force-pushed the feature/migrate-travis-to-github-actions branch from fad6d99 to 16e0ebb Compare December 23, 2022 08:09

MKodde approved these changes Jan 3, 2023

View reviewed changes

MKodde merged commit 072b2fe into develop Jan 4, 2023

MKodde deleted the feature/migrate-travis-to-github-actions branch January 4, 2023 07:18

MKodde mentioned this pull request Jan 5, 2023

Symfony 5 upgrade #565

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate travis github actions #571

Migrate travis github actions #571

Shaky212 commented Dec 21, 2022 •

edited

Loading

MKodde left a comment

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022 •

edited

Loading

MKodde Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 22, 2022

MKodde Dec 22, 2022

Shaky212 Dec 23, 2022

MKodde left a comment

Migrate travis github actions #571

Migrate travis github actions #571

Conversation

Shaky212 commented Dec 21, 2022 • edited Loading

MKodde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaky212 Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MKodde left a comment

Choose a reason for hiding this comment

Shaky212 commented Dec 21, 2022 •

edited

Loading

Shaky212 Dec 22, 2022 •

edited

Loading