Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

feat: install forge dependencies in postinstall #35

Merged
merged 6 commits into from
Jul 2, 2022
Merged

feat: install forge dependencies in postinstall #35

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a07d84
feat: install forge dependencies in postinstall
davisshaver Jul 1, 2022
15c08c5
feat: install foundry for app jobs too
davisshaver Jul 1, 2022
c9b5a60
feat: don't run forge postinstall in CI
davisshaver Jul 1, 2022
205db6c
feat: use github actions compatible syntax
davisshaver Jul 1, 2022
9e40f5a
feat: use logging to ensure no false return
davisshaver Jul 1, 2022
e56640c
feat: drop forge install from app action
davisshaver Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/app.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ jobs:
uses: actions/checkout@v3
with:
submodules: true
- name: Install Foundry
uses: onbjerg/foundry-toolchain@v1
with:
version: nightly
- name: Cache pnpm modules
uses: actions/cache@v3
with:
Expand Down Expand Up @@ -54,6 +58,10 @@ jobs:
uses: actions/checkout@v3
with:
submodules: true
- name: Install Foundry
uses: onbjerg/foundry-toolchain@v1
with:
version: nightly
Copy link

Choose a reason for hiding this comment

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

Needing to add this isn't a great side effect, why not add an environment variable to skip forge install rather? (or use the CI environment variable)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasoonS I like that idea – made this a draft for now, I will refactor the postinstall to not run forge install in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasoonS thanks again for the feedback, updated to reflect it!

Copy link
Owner

Choose a reason for hiding this comment

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

Given that this repo expects foundry (even if not used in the isolated context of the app package), I think this kind of change is fine/makes sense, and reflects what your local developer environment might have anyway.

We might consider adding this in as part of #36

- name: Cache pnpm modules
uses: actions/cache@v3
with:
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
},
"scripts": {
"preinstall": "npx -y only-allow pnpm",
"postinstall": "forge install",
"dev": "pnpm --parallel dev",
"prettier": "pnpm --parallel --no-bail prettier",
"lint": "pnpm --parallel --no-bail lint",
Expand Down