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

[TT-1326] Update Solidty Foundry pipeline with Slither #13986

Merged
merged 68 commits into from
Aug 9, 2024

Conversation

Tofel
Copy link
Contributor

@Tofel Tofel commented Aug 1, 2024

  • top level checks config
  • add Slither for modified files, excluding tests (in the future only for diff lines)
  • run code coverage for eligible products
  • run Forge fmt for eligible products

Running everything, when shared file changed: https://github.com/smartcontractkit/chainlink/actions/runs/10200971215
Running only one product: https://github.com/smartcontractkit/chainlink/actions/runs/10201139248

@Tofel Tofel marked this pull request as ready for review August 1, 2024 11:13
@Tofel Tofel requested review from a team and RensR as code owners August 1, 2024 11:13
product:
# 98.5 is the aspirational code coverage once we migrate all tests to Foundry
# product that have that minimum coverage of 98.5 in the matrix below are currently excluded from the check
- name: automation
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to move this to the top of the file, and also add the info if products have a gas snapshot, and if they use forge fmt? That would be a good single source of truth for all CI in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a really cool idea!

@@ -11,28 +11,55 @@ jobs:
name: Detect changes
runs-on: ubuntu-latest
outputs:
changes: ${{ steps.changes.outputs.src }}
src_changes: ${{ steps.changes.outputs.src }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to check for changes in the folders per product. Right now we would be running coverage for all products if any single one changes a line. Ideally, CI only runs for affected products.

Any changes to vendor or shared would trigger a full run, don't think we can get around that

run: |
forge fmt --check
SOL_FILES="${{ needs.changes.outputs.sol_files }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this over the extremely simple check we had before? This is lightning fast

chainchad
chainchad previously approved these changes Aug 8, 2024
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@Tofel Tofel enabled auto-merge August 9, 2024 09:29
@Tofel Tofel disabled auto-merge August 9, 2024 09:29
@Tofel Tofel enabled auto-merge August 9, 2024 09:46
@Tofel Tofel added this pull request to the merge queue Aug 9, 2024
Merged via the queue into develop with commit 4b91691 Aug 9, 2024
134 checks passed
@Tofel Tofel deleted the tt_1326_update_Sol_Foundry branch August 9, 2024 11:09
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.

5 participants