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

chore(ci/cd): switch deployment action #13545

Merged
merged 2 commits into from Jan 4, 2023
Merged

Conversation

guidoiaquinti
Copy link
Contributor

Changes

  1. remove all the container build logic that was present in .github/workflows/ci-e2e.yml as we now have dedicated workflows for it.

  2. move the Trigger PostHog Cloud deployment step from .github/workflows/ci-e2e.yml to .github/workflows/container-images-cd.yml.

  3. make the E2E CI Cypress tests pull the container image from the GitHub container registry instead of rebuilding it.

How did you test this code?

CI should be ✅ for .github/workflows/ci-e2e.yml. .github/workflows/container-images-cd.yml only runs on master so I didn't test it but it's more or less a copy paste of what we had already.

name: E2E CI

on:
pull_request:
push:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not need to run this on master anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's still sensible to run the tests on master - two branches may pass tests individually, but when both merged they may cause a failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's still sensible to run the tests on master - two branches may pass tests individually, but when both merged they may cause a failure

I agree and I'm 👍 to change this. I just want to point out that this PR didn't change the previous behaviour as the Cypress E2E tests matrix was only running on non-master

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh ok sure, makes sense then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A CI failure will anyway not block the deployment, is it ok for you?

run: echo "specs=$(ls cypress/e2e/* | jq --slurp --raw-input -c 'split("\n")[:-1] | _nwise(3) | join("\n")' | jq --slurp -c .)" >> $GITHUB_OUTPUT

cypress:
name: Cypress E2E tests (${{ strategy.job-index }})
if: ${{ github.ref != 'refs/heads/master' }} # Don't run on master, we only care about node_modules cache
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 necessary anymore as we do not run on master

@@ -13,7 +13,6 @@ on:
push:
branches:
- master
- main
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is coming from a great suggestion from @hazzadous: as we only have branch protection enabled on master, let's do not use main till we officially switch to it.

@@ -118,3 +117,23 @@ jobs:
# the posthog-cloud code we've checked out.
build-args: |
BASE_IMAGE=posthog/posthog:latest

- name: Trigger PostHog Cloud deployment
# TODO: switch to https://github.com/marketplace/actions/repository-dispatch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't change the action directly in this PR as there's likely a little dance that needs to happen between this repo and posthog-cloud-infra as the action output changes a bit.

@guidoiaquinti guidoiaquinti marked this pull request as ready for review January 3, 2023 14:39
@guidoiaquinti
Copy link
Contributor Author

Sorry @hazzadous but I had to fix a merge conflict. The conflict was a 1 liner

@hazzadous hazzadous self-requested a review January 4, 2023 10:45
@guidoiaquinti guidoiaquinti merged commit f08f76d into master Jan 4, 2023
@guidoiaquinti guidoiaquinti deleted the switch_deployment branch January 4, 2023 12:37
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

4 participants