Skip to content
This repository has been archived by the owner on May 22, 2023. It is now read-only.

CI job for Celo client #712

Merged
merged 18 commits into from
Mar 5, 2021
Merged

CI job for Celo client #712

merged 18 commits into from
Mar 5, 2021

Conversation

lukasz-zimnoch
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch commented Mar 2, 2021

Depends on: #710

Here we introduce the build-test-publish job for Celo. Its structure is almost identical than the existing build-test-publish job for Ethereum. Both jobs are now placed in separate client-celo.yml and client-ethereum.yml files. This is done in the sake of readability and allows easily grouping jobs under an appropriate namespace (Go / Celo and Go / Ethereum).

Regarding differences, the Celo job introduces additional build args (HOST_CHAIN=celo) while building Docker image and runs
client tests with celo build tag. Also, cache names in both jobs are intentionally differentiated to completely separate them as actions/cache action sets the cache scope to the key and branch. Something similar refers to the intermediary go-build-env images. They now have chain-specific suffixes to avoid problems while pushing them to local repo or cache in the future.

There is also a problem with reporting test results. @michalinacienciala noted a known bug (EnricoMi/publish-unit-test-result-action#12) in EnricoMi/publish-unit-test-result-action@v1 action we use to report test results. Because of that bug, it might happen that the test result summary will be under the wrong workflow while being presented in the PR summary. We're dropping the test results publishing step until the aforementioned bug gets resolved by the action maintainer.

Here we introduce the `build-and-test` job for
Celo. It's structure is almost identical than the
existing `build-and-test` job for Ethereum.
Both jobs are placed in separate `client-celo.yml`
and `client-ethereum.yml` files. This is done in
the sake of readability and allows easily grouping
jobs under appropriate namespace (Go / Celo
and Go / Ethereum). Regarding differences, the
Celo job introduces additional build args
(CHAIN=celo) while building Docker image and runs
client tests with `celo` build tag. Also, cache
names in both jobs are intentionally
differentiated to completely separate them as
`actions/cache` action sets the cache scope to
the key and branch. Something similar refers to
the intermediary `go-build-env` images. They now
have chain specific suffixes to avoid problems
while pushing them to local repo or cache in the
future.
@lukasz-zimnoch lukasz-zimnoch changed the base branch from multi-chain-dockerfile to master March 2, 2021 10:39
@lukasz-zimnoch lukasz-zimnoch changed the base branch from master to multi-chain-dockerfile March 2, 2021 10:39
Base automatically changed from multi-chain-dockerfile to master March 3, 2021 10:04
@lukasz-zimnoch lukasz-zimnoch marked this pull request as ready for review March 3, 2021 10:21
Comment on lines 7 to 9
pull_request:
branches:
- master
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the filter and run iyt on all PRs.

Suggested change
pull_request:
branches:
- master
pull_request:

Copy link
Member Author

Choose a reason for hiding this comment

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

Please see: 03aafa3

.github/workflows/client-celo.yml Outdated Show resolved Hide resolved
.github/workflows/client-celo.yml Outdated Show resolved Hide resolved
.github/workflows/client-celo.yml Outdated Show resolved Hide resolved
.github/workflows/client-ethereum.yml Outdated Show resolved Hide resolved
Also, got rid of chain-specific suffixes from
test results directories and files.
@lukasz-zimnoch
Copy link
Member Author

@michalinacienciala all comments addressed!

nkuba
nkuba previously approved these changes Mar 5, 2021
# VERSION= ? TODO: Configure version, sample: 1.7.6
push: |
${{ startsWith(github.ref, 'refs/heads/releases/alfajores') }}
Copy link
Member

Choose a reason for hiding this comment

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

We could simplify branch name check with:

${{ startsWith(github.head_ref, 'releases/alfajores') }}

But it's something that I'd not block this PR with.

cc: @michalinacienciala

@nkuba
Copy link
Member

nkuba commented Mar 5, 2021

LGTM leaving final approval to @michalinacienciala

@michalinacienciala michalinacienciala merged commit 6ad6e61 into master Mar 5, 2021
@michalinacienciala michalinacienciala deleted the celo-ci-jobs branch March 5, 2021 11:31
@pdyraga pdyraga added this to the v1.8.0 milestone Mar 5, 2021
@nkuba nkuba removed this from the v1.8.0 milestone Sep 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants