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

Conversation

davisshaver
Copy link
Contributor

@davisshaver davisshaver commented Jul 1, 2022

Installs forge dependencies in postinstall

Screen Shot 2022-07-01 at 6 04 45 PM

- 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

@davisshaver davisshaver marked this pull request as draft July 1, 2022 21:32
@davisshaver davisshaver marked this pull request as ready for review July 1, 2022 22:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants