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

Start phasing out CircleCI #709

Merged
merged 4 commits into from
Aug 17, 2020
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Aug 17, 2020

GitHub Actions, first introduced in March (#600), has been more or less stable alternative to CircleCI. In fact, by observations show that both GHA and CircleCI have about the same rate of spurious failures (around 15 broken builds per month, due to various reasons).

Most of the jobs performed by CircleCI are duplicated on GitHub Actions. However, CircleCI has subpar capabilities of limiting the build impact. Currently, every changeset triggers the entire CircleCI build which takes ages (~30–40 minutes). CircleCI does not have built-in support for triggering builds only on some files, as opposed to GitHub Actions.

Given this, I suggest to phase out CircleCI in favor of GitHub Actions, arguably better integrated with GitHub. This PR removes the parts of CircleCI configuration which are already replaced with GitHub Actions. Eventually, the remaining bits will be migrated to GitHub Actions too and CircleCI configuration can be removed completely.

Checklist

  • Change is covered by automated tests
  • Changelog is updated

Remove jobs that have been completely replaced by GitHub Actions.

  - "android" replaced with "android-tests" from "test-java.yaml".

  - "analyze" replaced with "check-formatting" from "code-style.yaml"
    and "sanitizers" from "test-core.yaml".

  - "benchmark" replaced with "benchmarks" from "test-core.yaml".

  - "fuzz" replaced with "fuzzing" from "test-core.yaml".

  - "gothemis" replaced with "unit-tests" from "test-go.yaml".

  - "jsthemis" replaced with "unit-tests" from "test-{nodejs,wasm}.yaml".

  - "integration_tests" replaced with "cross-language" from "integration.yaml".

  - "php5", "php70", "php71" replaced with "unit-tests" from "test-php.yaml".
Drop tasks from the "x86_64" megajob which have been replaced by
individual jobs on GitHub Actions.

  - ThemisPP is tested by "unit-tests" in "test-cpp.yaml".

  - JNI library tested by "unit-tests" in "test-java.yaml".

  - PyThemis is tested by "unit-tests" in "test-python.yaml".

  - RbThemis is tested by "unit-testt" in "test-ruby.yaml". There is no need
    to install RVM if this gets removed.

  - RustThemis is tested by "unit-tests" in "test-rust.yaml". There is no
    need to install Rust toolachain too.

  - Builds with Valgrind are tested by "leak-check" in "test-core.yaml".

  - Themis Core (include Secure Cell compatibility) is tested by
    "unit-tests" in "test-core.yaml".

There is no need to install so many packages either. However, some of
them are still necessary to build Themis Core for coverage testing.
@ilammy ilammy added the infrastructure Automated building and packaging label Aug 17, 2020
Copy link
Contributor

@iamnotacake iamnotacake left a comment

Choose a reason for hiding this comment

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

Seems like CircleCI will have much less work to do.

This PR removes the parts of CircleCI configuration which are already replaced with GitHub Actions

So even Android was already tested by GHA? Didn't notice that before. Well, maybe because GHA skips testing my non-Android-related changes on Themis.

And I see you cleaned x86_64 -> docker -> cossacklabs/android-build:2019.01 so it's only doing code coverage tests now?

@ilammy
Copy link
Collaborator Author

ilammy commented Aug 17, 2020

@iamnotacake,

Well, maybe because GHA skips testing my non-Android-related changes on Themis.

Yes, unless you touch anything that can affect Android builds, those builds are not started. For example, note how here you have Android emulator runs because Themis Core is changed, but if only Rust core is changed then Android tests are not triggered.

Additionally, regardless of the changeset, full test suite is run after any PR is merged into master or any of the release branches.

so it's only doing code coverage tests now?

That's right. This is the only thing it does now.

@vixentael
Copy link
Contributor

Let's see how it goes.
Also, @ilammy would you mind out GHA badge to the Readme? Lowering # of CircleCI builds suggests to make GHA CI visible as well.

@vixentael
Copy link
Contributor

Also, we might need to update "branch protection rules" settings to mark some GHA jobs as required.

@ilammy
Copy link
Collaborator Author

ilammy commented Aug 17, 2020

Also, @ilammy would you mind out GHA badge to the Readme?

Yeah, good idea!

Though, GHA badges are a bit different since their suite is split into multiple "workflows" and badges are per-workflow. So guess the best way would be to put integration testing badge up top:

Integration testing

Since this workflow is the closest one to testing everything at once.

Or we can use the Core one:

Themis Core

And maybe add per-languages badges down in the language table:

GoThemis
JavaThemis
ObjCThemis

What do you think?

Also, we might need to update "branch protection rules" settings to mark some GHA jobs as required.

Here we need to be careful to not shoot ourselves into the foot. See how this PR requires some CircleCI jobs to pass which will never complete? I don't want documentation PRs to be blocked if they don't trigger any GHA jobs... Well, I guess, there is only one way to find out. Yep, just added Android emulator to the required list, now it's blocking this PR as well. Not sure what's the best way here. I guess we can just leave them not required? Somehow add a check that checks other checks and make it required?..

@vixentael
Copy link
Contributor

@ilammy great suggestions! Let's use maximum :)

Let's put Cross-platform tests Themis Core tests to the top.

And then language-dependent badges to the language table.


See how this PR requires some CircleCI jobs to pass which will never complete?

Exactly.. Looks like we already need to fix branch protection rules.

Since we're switching to GitHub Actions as a primary indicator of CI
health, add some badges for them. Since there is no single omnibadge for
the entire test suite, add multiple one for various aspects.

Since there are many badges there, they don't really fit in line. Move
them all into the second row.
@ilammy
Copy link
Collaborator Author

ilammy commented Aug 17, 2020

And then language-dependent badges to the language table.

I tried fitting them in, but the horizontal space must be very expensive there. However I try to jam them into that table, they just don't look pretty. So maybe we don't need them.

Even badges for GHA take a bunch of space so I had to move them to the second line so that it looks nicer.

@vixentael
Copy link
Contributor

vixentael commented Aug 17, 2020 via email

@ilammy
Copy link
Collaborator Author

ilammy commented Aug 17, 2020

Well, as for required checks, I don't have a good solution at the moment. For now, I have lifted the requirement from most of them. There is an internal task tracking this issue: T1759. I hope some day GHA checks can be made required.

@ilammy ilammy merged commit 35ac93e into cossacklabs:master Aug 17, 2020
@ilammy ilammy deleted the phase-out-circleci branch August 17, 2020 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Automated building and packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants