Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

chore: migrate to GitHub Actions for CI #3701

Merged
merged 4 commits into from
Oct 14, 2021
Merged

chore: migrate to GitHub Actions for CI #3701

merged 4 commits into from
Oct 14, 2021

Conversation

devoto13
Copy link
Collaborator

@devoto13 devoto13 commented Oct 1, 2021 •
edited by jginsburgn
Loading

  • Use headless browsers instead of xfvb where possible.
  • Replace commitlint-travis with usage of the base commitlint as there is no official CLI for GitHub Actions.
  • Remove CI badges as GitHub displays the CI status natively for each commit.

Remaining work (for which I don't have permissions):

@devoto13 devoto13 requested a review from jginsburgn October 1, 2021 19:46
@jginsburgn
Copy link
Member

Why do we want to use headless browsers instead of xvfb?

@devoto13
Copy link
Collaborator Author

devoto13 commented Oct 13, 2021 •
edited
Loading

@jginsburgn I think xvfb was used mostly for historical reasons because Karma predates times when the headless mode was implemented by browsers.

It seems that xvfb requires extra work or a third-party dependency to use it in GitHub Actions, so I decided to get rid of it altogether. Headless should work the same as headful for the purpose of running these tests and we test in multiple browsers in headful mode using BrowserStack anyways.

@jginsburgn
Copy link
Member

How can we protect ourselves from malicious executions triggered by pushes and PRs?

@devoto13
Copy link
Collaborator Author

As it says in the PR description, secrets are not available to the builds run from forks (i.e. when somebody forks repo and sends a PR). So I don't think there is any security risk there.

As for the malicious pushes... Well, we should not give push access to the people we don't trust. But that's pretty much the same level of security as we used to have with Travis. I'm not sure how we can make it better without hurting the ability of collaborators to actually do work (e.g. run tests on BrowserStack to debug issues, etc).

jginsburgn
jginsburgn reviewed Oct 14, 2021
View reviewed changes
jginsburgn
jginsburgn reviewed Oct 14, 2021
View reviewed changes
@jginsburgn
Copy link
Member

Remove/disable Travis/AppVeyor?

I have deleted the Travis webhooks. Can you clarify what is missing for Travis?

@jginsburgn
Copy link
Member

BROWSER_STACK_USERNAME
BROWSER_STACK_ACCESS_KEY

Shouldn't we use BROWSER_STACK_ACCESS_KEY instead?

@jginsburgn
Copy link
Member

SAUCE_USERNAME
SAUCE_ACCESS_KEY

Where and how are these used?

@jginsburgn
Copy link
Member

NPM_TOKEN

Just to confirm, this is only used in the "Release" workflow by the npm run semantic-release step, right?

@jginsburgn
Copy link
Member

GH_TOKEN (not 100% if this one is needed with GitHub Actions, I guess we can try without it and see)

I think this is not needed :)

@devoto13
Copy link
Collaborator Author

devoto13 commented Oct 14, 2021 •
edited
Loading

I'll try to close/re-open PR to see if GitHub Actions get triggered.

UPD Does not seem so. Will fix other comments and then see how to solve it then.

@devoto13 devoto13 closed this Oct 14, 2021
@devoto13 devoto13 reopened this Oct 14, 2021
@devoto13
Copy link
Collaborator Author

devoto13 commented Oct 14, 2021 •
edited
Loading

I have deleted the Travis webhooks. Can you clarify what is missing for Travis?

I'll open a new PR to confirm that Travis and AppVeyor are not triggered to see if that's enough or we need to do something else.

UPD

#3702

image

So looks like Travis was still triggered for the branch. Maybe it's configured somewhere in the settings as a required check or something?

Also, Appveyor was run, so we need to disable it as well.

Maybe https://remarkablemark.medium.com/remove-travis-ci-from-github-7ea674784897?

Shouldn't we use BROWSER_STACK_ACCESS_KEY instead?

I don't understand. What exactly do you mean by this?

Where and how are these used?

SuaceLabs tokens are used in the integration tests, which are run by npm run test:integration.

Just to confirm, this is only used in the "Release" workflow by the npm run semantic-release step, right?

That's correct.

I think this is not needed :)

That's actually used by update-docs.js to update documenation when release is cut. I would suggest putting the same token as we have in Travis to secrets as it should have push access to the documentation website repo. Or alternatively, we can generate a new token specifically for that repo and store it under a different name (DOCS_GITHUB_TOKEN or something). Let me know if you would prefer the second solution?

@jginsburgn jginsburgn mentioned this pull request Oct 14, 2021
Open
devoto13 and others added 3 commits October 14, 2021 23:12
@devoto13
chore: migrate to GitHub Actions for CI
6cf0d62 
- Use headless browsers instead of xfvb where possible.
- Replace commitlint-travis with usage of the base commitlint as there is no official CLI for GitHub Actions.
- Remove CI badges as GitHub displays the CI status natively for each commit.
@devoto13
chore: use BS plugin's compatible env names
046f8b6 
See: https://github.com/karma-runner/karma-browserstack-launcher#:~:text=username%20your%20BS,BROWSERSTACK_ACCESS_KEY%20env%20variable.
@devoto13
chore: improve GH Actions workflow's job names
0ad4ab8 
This is needed because in GH's configuration, we need to distinguish the required checks for protected branches and giving them unique names results in easiera configs.
@devoto13
chore: remove client testing from the windows job of test.yml
a050f4c 
The purpose of this test is to make sure that client JS works in different browsers and it does not need to be run on different OS/Node versions.
@jginsburgn jginsburgn merged commit 9a99189 into karma-runner:master Oct 14, 2021
@devoto13 devoto13 deleted the github-actions branch October 16, 2021 12:24
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.3.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sync-by-unito sync-by-unito bot mentioned this pull request Nov 1, 2021
Closed
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
@devoto13 @anthony-redFox
chore: migrate to GitHub Actions for CI (karma-runner#3701)
d0ca2c6 
* chore: migrate to GitHub Actions for CI

- Use headless browsers instead of xfvb where possible.
- Replace commitlint-travis with usage of the base commitlint as there is no official CLI for GitHub Actions.
- Remove CI badges as GitHub displays the CI status natively for each commit.

* chore: use BS plugin's compatible env names

See: https://github.com/karma-runner/karma-browserstack-launcher#:~:text=username%20your%20BS,BROWSERSTACK_ACCESS_KEY%20env%20variable.

* chore: improve GH Actions workflow's job names

This is needed because in GH's configuration, we need to distinguish the required checks for protected branches and giving them unique names results in easiera configs.

* chore: remove client testing from the windows job of test.yml

The purpose of this test is to make sure that client JS works in different browsers and it does not need to be run on different OS/Node versions.

Co-authored-by: Jonathan Ginsburg <jginsburgn@google.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jginsburgn jginsburgn jginsburgn approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@devoto13 @jginsburgn @karmarunnerbot