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

Move migration and publishing of contracts (Ethereum) to GH Actions #2376

Merged

Conversation

michalinacienciala
Copy link
Contributor

As described in RFC-18, there is a need for a refactorization of Keep
and tBTC release processes in order to reduce human involvement and
make the processes faster and less error prone. A part of this task is
migration from CircleCI jobs to GitHub Actions. This commit enhances
the workflow for building and testing Solidity with a job migrating
contracts and publishing them to the npm registry. Contracts also get
pushed to Tenderly and uploaded to Google Cloud Storage. For now, only
keep-test configuration is implemented. Implementation for other
environments will be done at a later stage of work on RFC-18.

The migrate-and-publish-ethereum job is configured to execute only if
workflow is started by a push or workflow_dispatch event on master or
rfc-18/* branch. The rfc-18/* condition will need to be removed
once whole build-test-migrate-publish-keep-test CircleCI process
gets migrated to GH Actions.
Additionally, as we may not always want to migrate and publish
contracts on every push (or workflow_dispatch) on master, the job was
configured to require manual approval in GitHub GUI. This is done by
configuration of environment protection rules on keep-test and using
environment: keep-test config in the workflow.

As described in RFC-18, there is a need for a refactorization of Keep
and tBTC release processes in order to reduce human involvement and
make the processes faster and less error prone. A part of this task is
migration from CircleCI jobs to GitHub Actions. This commit enhances
the workflow for building and testing Solidity with a job migrating
contracts and publishing them to the npm registry. Contracts also get
pushed to Tenderly and uploaded to Google Cloud Storage. For now, only
keep-test configuration is implemented. Implementation for other
environments will be done at a later stage of work on RFC-18.

The `migrate-and-publish-ethereum` job is configured to execute only if
workflow is started by a push or workflow_dispatch event on `master` or
`rfc-18/*` branch. The `rfc-18/*` condition will need to be removed
once whole `build-test-migrate-publish-keep-test` CircleCI process
gets migrated to GH Actions.
Additionally, as we may not always want to migrate and publish
contracts on every push (or workflow_dispatch) on `master`, the job was
configured to require manual approval in GitHub GUI. This is done by
configuration of environment protection rules on `keep-test` and using
`environment: keep-test` config in the workflow.
@michalinacienciala
Copy link
Contributor Author

michalinacienciala commented Mar 10, 2021

Here is an example of passing migrate-and-publish-ethereum job (on different branch, but containing the changes introduced in this PR). In that example Push contracts to Tenderly optional step is failing. Since then there were changes to the keep-network/tenderly-push-action action - we need to check if the step is passing after those changes.

EDIT: migrate-and-publish-ethereum job is currently failing due to bug in truffle (trufflesuite/truffle#3913).
EDIT 2: Bug in truffle/geth is now fixed, but Push contracts to Tenderly [is still failing[(https://github.com/keep-network/keep-core/runs/2076626474?check_suite_focus=true) - investigation of root cause is in progress.

@michalinacienciala michalinacienciala changed the title Move migration and publishing of contracts to GH Actions Move migration and publishing of contracts (Ethereum) to GH Actions Mar 10, 2021
@michalinacienciala michalinacienciala temporarily deployed to keep-test March 11, 2021 13:37 Inactive
@michalinacienciala
Copy link
Contributor Author

@lukasz-zimnoch, @nkuba - migration and push to Tenderly are finally working (see run #650) 🎉 You can review and merge if no changes needed.

@michalinacienciala michalinacienciala force-pushed the rfc-18/ropsten/move-migrate-and-publish-solidity-contracts branch from 4e51165 to e383858 Compare March 11, 2021 15:13
@michalinacienciala michalinacienciala force-pushed the rfc-18/ropsten/move-migrate-and-publish-solidity-contracts branch from e383858 to 816c129 Compare March 11, 2021 16:06
Name of the secret storing private key for the account used for
migration of contracts on Ethereum has been changed, so that we would
have similar naming to that for Celo chain account.
Also names of the ETH_HOSTNAME, ETH_NETWORK_ID and TRUFFLE_NETWORK
secrets have been enhanced with `KEEP_TEST` / `KEEP_TEST_ETH` prefix.
@michalinacienciala michalinacienciala force-pushed the rfc-18/ropsten/move-migrate-and-publish-solidity-contracts branch from 816c129 to f24cd6e Compare March 11, 2021 16:49
@michalinacienciala michalinacienciala temporarily deployed to keep-test March 11, 2021 16:59 Inactive
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch left a comment

Choose a reason for hiding this comment

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

Looks good to me! Leaving final approval for @nkuba

Copy link
Member

@nkuba nkuba left a comment

Choose a reason for hiding this comment

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

Good to merge. The comments I left can be handled in another PR.

&& (github.event_name == 'push'
|| github.event_name == 'workflow_dispatch')
# TODO: Implement similiar for keep-dev (should execute for rfc-18... & master branch)
environment: keep-test # currently keep-test requires manual aproval of job
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
environment: keep-test # currently keep-test requires manual aproval of job
environment: keep-test # keep-test requires a manual approval

|| github.ref == 'refs/heads/master')
&& (github.event_name == 'push'
|| github.event_name == 'workflow_dispatch')
# TODO: Implement similiar for keep-dev (should execute for rfc-18... & master branch)
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 this comment, we won't be adding keep-dev at the moment.

ETH_HOSTNAME: ${{ secrets.KEEP_TEST_ETH_HOSTNAME }}
CONTRACT_OWNER_ETH_ACCOUNT_PRIVATE_KEY: ${{ secrets.KEEP_TEST_ETH_CONTRACT_OWNER_PRIVATE_KEY }}
run: npx truffle migrate --reset --network $TRUFFLE_NETWORK # writes artifacts to /home/runner/work/keep-core/keep-core/solidity/build/contracts
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the # writes artifacts to /home/runner/work/keep-core/keep-core/solidity/build/contracts comment?

Comment on lines +183 to +191
# (requires some changes, as below config throws an error)
# - name: Publish to npm
# uses: JS-DevTools/npm-publish@v1
# with:
# package: ./solidity/package.json
# token: ${{ secrets.NPM_TOKEN }}
# access: public
# dry-run: true # TODO: to be removed once testing of workflow is done
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 this whole part, we will stick with the current one for now.

@nkuba nkuba merged commit ef6f0c5 into master Mar 12, 2021
@nkuba nkuba deleted the rfc-18/ropsten/move-migrate-and-publish-solidity-contracts branch March 12, 2021 16:09
@michalinacienciala
Copy link
Contributor Author

Good to merge. The comments I left can be handled in another PR.

Thanks for the input :) I applied the proposed changes as part of this cleanup PR: #2378.

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.

3 participants