celo-org / celo-monorepo

Official repository for core projects comprising the Celo platform
https://celo.org
Apache License 2.0
684 stars 360 forks source link

refactor: reads supported Foundry version from an environment variable #11046

Closed arthurgousset closed 6 days ago

arthurgousset commented 2 weeks ago

Description

  1. reads an environment variable (SUPPORTED_FOUNDRY_VERSION) defined at the celo-org (GitHub org) level.
  2. updates the Foundry installation version to use the defined variable for consistency across workflows in celo-org/celo-monorepo and celo-org/developer-tooling.

The overall objective is to update Foundry versions in lock-steps. See more context on pros and cons in:

Other changes

None.

Tested

Tested on CI. This only changes CI workflows.

Related issues

Backwards compatibility

Yes.

Documentation

Yes in code comments and this PR description.

arthurgousset commented 2 weeks ago

Expect the workflows to fail, because the SUPPORTED_FOUNDRY_VERSION is not yet defined at the celo-org (GitHub org) level. Context https://github.com/celo-org/celo-monorepo/issues/11032#issuecomment-2178542976.

arthurgousset commented 2 weeks ago

Expect the workflows to pass, because the SUPPORTED_FOUNDRY_VERSION is defined at the celo-org (GitHub org) level. See Slack confirmation.

arthurgousset commented 2 weeks ago

Waiting to confirm whether I should merge this branch against master or release/core-contracts/12 .

arthurgousset commented 2 weeks ago

Converting this into a draft PR again. I'll merge this branch into release/core-contracts/12, but I need to cherry-pick my commits, and cut a new branch from release/core-contracts/12 that doesn't include the latest mento commits against master.

arthurgousset commented 2 weeks ago

That should be done with https://github.com/celo-org/celo-monorepo/pull/11046/commits/8f039a20fdadee41c15dfea5c4023e00234c0de6. Waiting for CI to pass before marking this as ready for review.

arthurgousset commented 2 weeks ago

CI didn't complete, because runner didn't pick the "Build & Integration Tests / Install dependencies" workflow. Cancelled, and re-running failed jobs.

arthurgousset commented 2 weeks ago

All workflows are green, but waiting for "Protocol Compatibility" to report. Not sure why this is stuck at the moment.

image
arthurgousset commented 2 weeks ago

When I run the "Protocol Compatibility" command locally, the command passes:

$ yarn
$ yarn build
$ yarn --cwd packages/protocol test compatibility/

# ...
  76 passing (4s)
  2 pending

✨  Done in 196.13s.

Context:

https://github.com/celo-org/celo-monorepo/blob/c870d4973442bf1322a0cc51bd26b6b4f79e2cca/.github/workflows/celo-monorepo.yml#L220-L222

openzeppelin-code[bot] commented 1 week ago

refactor: reads supported Foundry version from an environment variable

Generated at commit: e886bb107683aa146135035501c76583dbf748e3

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
3
0
15
41
61
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

arthurgousset commented 1 week ago

The problem with the missing (or "expected") workflow was that the condition that is evaluated before running the steps could never evaluate to true here, because it was running against release/core-contracts/** branches.

Now that the condition includes release branches, this step can run:

https://github.com/celo-org/celo-monorepo/blob/3a0a771cac2917a703f48a02e5a0859bb0e62344/.github/workflows/celo-monorepo.yml#L160-L164

The required changes was:

- github.base_ref == 'master' || contains(github.base_ref, 'production') ||
+ github.base_ref == 'master' || contains(github.base_ref, 'release') || contains(github.base_ref, 'production') ||

This was fixed in https://github.com/celo-org/celo-monorepo/pull/11080, which was merged in the meantime.

The workflows are all green now ✅