The CI has become so confusing

[AbdulrhmnGhanem]

The name of the jobs isn't the name of the commits. Also, it keeps merging unrelated branches into review for example while working on #281, the action running the last changes done in the last commit is

Merge branch 'fix-logo-blink' of https://github.com/kitspace/kitspace-v2 Docker #1409

not action with commit name.

I think the previous workflow was much better we only need to add merging a PR into review whenever it's marked as ready for review. And I don't know what triggers a new build now, #276 is failing on a deleted test! I think this needs to get fixed before merging other PRs.

[kasbah]

I changed two major things about the CI:

1. Using pull_request_target trigger in all workflows.

• I think this is confusing you the most. This triggers the CI when pull-requests are made to the master branch.
• Unlike pull_request it has the same security privileges as push events (push runs on push to a branch -- what we were using before) but pull_request_target also runs the workflow file that is on master not the pull-request branch. (There's a longer article from Github on this that might help).
• (I guess this also changes the worflow run title in the Action pane -- it uses the pull request title rather than the commit. Not sure why.)
• This does makes debugging/working on the workflow itself a bit harder. (One trick is to add the branch you are currently working on to the on: push array and remove it again when it's ready -- you still get failures from the pull_request_target runs but at least you can confirm it works)
• The reason I made this change is so that Dependabot pull-requests can run our CI without getting errors about missing secrets.

2. The other aspect is the review branch merging. Yes, it merges unrelated branches into review and deploys them. But it's as you say: only branches that are marked ready for review. (So I'm not clear on what you want to change there). The effect of this is that you can basically ignore all the CI runs with the review tag and just rely on the "github-actions bot deployed to review.staging.kitspace.dev" statuses on the PRs.

The two things are unrelated. So we can still change one without affecting the other, but it seems you are confused because I changed two major things in a short time span. I would say: give it some time and if you still find it annoying after changing your mental model about what's happening then we can look at changing the pull_request_target strategy but still avoid Dependabot PRs failing.

[AbdulrhmnGhanem]

I don't understand this one: in 3a73e49 the pull_request_target is failing because it's running a deleted test! What may cause this?

[kasbah]

I'm not sure. I assumed it would be because something was deleted from docker.yml but it's running the docker.yml from master but that is not the case?

[kasbah]

Sorry, I figured it out. This checkout:

kitspace-v2/.github/workflows/docker.yml

Lines 232 to 235 in a9d50f0

	- name: Check out the repo
	uses: actions/checkout@v2
	with:
	submodules: 'recursive'

Needs to be changed to be like this checkout (without the if though):

kitspace-v2/.github/workflows/docker.yml

Lines 86 to 94 in a9d50f0

	- name: Check out the repo
	uses: actions/checkout@v2
	if: steps.check_for_image.outputs.image_exists == 'false'
	with:
	# check out the pull-request when triggered by "pull_request_target",
	# when triggered on "push" this is empty and checks out the branch
	# pushed to
	ref: ${{ github.event.pull_request.head.sha }}
	submodules: 'recursive'

Needs to be changed on the master branch docker.yml to have an effect on your runs.

EDIT: I changed it: e6c4f2f

[AbdulrhmnGhanem]

I think the review workflow shouldn't run the e2e especially after enabling running tests in parallel any run of the e2e will spin up 12 jobs (GitHub Actions free plan supports 20 concurrent containers).

[kasbah]

I've been thinking maybe we should stop testing across different browsers for the e2e tests. Seems like a lot of repeated tests that are not really about browser compatibility. Functional tests on a component level seem like a better target for cross-browser tests.

How come it's 12 now anyway? What's this new containers matrix about?

kitspace-v2/.github/workflows/docker.yml

Lines 223 to 225 in 3123cb2

	matrix:
	browser: [chrome, edge, electron, firefox]
	containers: [1, 2, 3]

[AbdulrhmnGhanem]

How come it's 12 now anyway? What's this new containers matrix about?

Running the tests in parallel.

[kasbah]

But [1, 2, 3] are just repeating the same tests on the same browser?

[AbdulrhmnGhanem]

But [1, 2, 3] are just repeating the same tests on the same browser?

No, Cypress uses groups to divide the workload between different machines. It will find 3 machines belonging to Chrome for example, so it divides the workload across these three machines.

kitspace-v2/.github/workflows/docker.yml

Lines 300 to 303 in 3123cb2

	parallel: true
	record: true
	browser: ${{ matrix.browser }}
	group: ${{ matrix.browser }}