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

Migrate to GitHub Actions #160

Merged
merged 4 commits into from
Nov 4, 2020

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Oct 20, 2020

Reviewers: @usabilla/oss-docker

Please specify the type of changes being proposed:

Q A
Documentation? no
Dockerfile change? no
Build feature? yes
Apply CVE Patch? no
Remove CVE Patch? no

Changes

This PR replaces the current Circle CI pipeline with a GitHub Action workflow. This is a table with all the changes to each step changed:

Circle CI GitHub Actions Changes
lint-shell lint-shell N/A
lint lint-docker N/A
build-prometheus-exporter-file build-prometheus-exporter-file N/A
build-http build-http The builds of ngnix images now run in parallel
build-fpm build-php The builds of PHP FPM images now run in parallel
build-cli build-php The builds of PHP CLI images now run in parallel
scan-vulnerability scan-vulnerability-(php/http/prometheus-exporter-file) Now the steps to scan images' vulnerabilities are split
test-cli test-php Now test-cli and test-fpm run in the same step, but in parallel
test-fpm test-php Now test-cli and test-fpm run in the same step, but in parallel
test-http test-http Now http-test and test-http-e2e run in the same step, across all versions of Alpine, Ngnix and PHP
test-http-e2e test-http Now make http-test and make test-http-e2e run in the same step, across all versions of Alpine, Ngnix and PHP
test-prometheus-exporter-file-e2e test-prometheus-exporter-file N/A
push-approval N/A This is done automatically by GitHub Actions
push-cli push-php Now push-cli and push-fpm run in the same step, but in parallel
push-fpm push-php Now push-cli and push-fpm run in the same step, but in parallel
push-http push-http N/A
push-prometheus-exporter-file push-prometheus-exporter-file N/A

Benchmarks

CircleCI takes usually 30min or more to run all the steps, excluding the pushing of the images. Examples:

GitHub Actions usually take 15min or less to run all the steps, including the new ones, and excluding the pushing of the images. Some examples:

TODOs

  • Drop Circle CI requirements from GitHub's master branch protection

@WyriHaximus WyriHaximus added the Documentation This issue or pull request is related to documentation and written guidelines label Oct 20, 2020
@WyriHaximus WyriHaximus force-pushed the story/GFPPLAT-451/migrate-to-github-actions branch 29 times, most recently from 9f0d321 to 45c3c47 Compare October 21, 2020 11:50
@carusogabriel carusogabriel requested review from rdohms and removed request for fabiotc, frankkoornstra and agustingomes October 30, 2020 16:05
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
needs:
- lint-docker
- lint-shell
runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also use yaml native templating, so you could avoid some of the repetitions like this one

.github/workflows/ci.yml Show resolved Hide resolved
@carusogabriel carusogabriel force-pushed the story/GFPPLAT-451/migrate-to-github-actions branch from 77676f0 to 8bf7393 Compare November 2, 2020 12:32
@WyriHaximus WyriHaximus force-pushed the story/GFPPLAT-451/migrate-to-github-actions branch 7 times, most recently from d19e248 to 4487069 Compare November 3, 2020 14:21
Copy link
Contributor

@carusogabriel carusogabriel left a comment

Choose a reason for hiding this comment

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

That's huge 🚀

Copy link
Contributor

@cvmiert cvmiert left a comment

Choose a reason for hiding this comment

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

🚢

@WyriHaximus WyriHaximus force-pushed the story/GFPPLAT-451/migrate-to-github-actions branch from 708825f to dd1c771 Compare November 4, 2020 12:36
@WyriHaximus WyriHaximus merged commit 29ea4bb into master Nov 4, 2020
@WyriHaximus WyriHaximus deleted the story/GFPPLAT-451/migrate-to-github-actions branch November 4, 2020 13:14
- run: sudo chown -R 1000:1000 ./test/functional/web/tmp/ # Ensure we have the same uid:gid as our `app` docker user
shell: bash
- run: make test-prometheus-exporter-file-e2e
check-mark: # This is our required step, pay extra attention when this step is changed for what ever reason
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have branch protection on the main branch, and instead of having to check/uncheck 60 required checks when we add/remove new PHP versions we only make this job required as it requires the previous jobs to pass before it even runs. This way we don't have to update the required checks when a job's name changes or a new version comes into play, or when we consolidate all the build jobs into one.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i suggest using https://github.com/probot/settings for managing that stuff, not manually clicking in the UI. HTH!

Copy link
Member Author

Choose a reason for hiding this comment

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

That could work, but it would mean updating the settings file and the naming/versions in two different PR as AFAIK the settings need to be applied/merged before they are effective on the PR. (I do love that suggestions, using it for my personal repo's to managed settings 👍 .)

Copy link
Contributor

Choose a reason for hiding this comment

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

that is true - but we do the update in one PR and just have an admin merge :)

Copy link
Member Author

Choose a reason for hiding this comment

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

That is also a possibility. But what I would like even more is that they let us select the underlying job name instead of the fancy user friendly name, that way you only have to select a handful instead of nearly a 100 🤐 . brb doing a feature suggestion to GitHub for that

WyriHaximus added a commit that referenced this pull request Feb 22, 2021
During the migration to GitHub Actions in #160 this functionality
was mistakenly and overzealously removed. Since PHP 8 and
Alpine 3.13 are out and #166 has been filed, currently with a CVE
for musl in it, this check should have failed as it is our goal
to ship images without known CVE's in it. On my own PHP images
the CVE checking fails and as such I was surprised that #166
didn't have any failures. Up on checking the CI logs it showed
the musl CVE but the step didn't fail.

This commit restores the original functionality and will make the
CI once again fail when it finds a CVE in one of the images.
WyriHaximus added a commit that referenced this pull request Feb 25, 2021
During the migration to GitHub Actions in #160 this functionality
was mistakenly and overzealously removed. Since PHP 8 and
Alpine 3.13 are out and #166 has been filed, currently with a CVE
for musl in it, this check should have failed as it is our goal
to ship images without known CVE's in it. On my own PHP images
the CVE checking fails and as such I was surprised that #166
didn't have any failures. Up on checking the CI logs it showed
the musl CVE but the step didn't fail.

This commit restores the original functionality and will make the
CI once again fail when it finds a CVE in one of the images.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI This issue or pull request is related to the build process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to Github Actions
6 participants