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

Add profile input in build workflow #7

Closed
andreivladbrg opened this issue Apr 9, 2024 · 6 comments · Fixed by #27
Closed

Add profile input in build workflow #7

andreivladbrg opened this issue Apr 9, 2024 · 6 comments · Fixed by #27
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: feature New feature or request. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.

Comments

@andreivladbrg
Copy link
Member

andreivladbrg commented Apr 9, 2024

It would be useful if we can choose the profile we build the contracts in the forge build workflow

Similar to what we do in forge test workflow:

FOUNDRY_PROFILE: ${{ inputs.foundry-profile }}

@andreivladbrg andreivladbrg added effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: refactor Change that neither fixes a bug nor adds a feature. type: feature New feature or request. work: clear Sense-categorize-respond. The relationship between cause and effect is clear. and removed type: refactor Change that neither fixes a bug nor adds a feature. labels Apr 9, 2024
@PaulRBerg
Copy link
Member

Gud idea

@smol-ninja smol-ninja self-assigned this Oct 21, 2024
@smol-ninja smol-ninja removed their assignment Oct 31, 2024
@smol-ninja
Copy link
Member

@andreivladbrg why do we need this? We never use any other profile than optimized in CI?

@andreivladbrg
Copy link
Member Author

I don't remember exactly the motivation - but I can assume it's about optionality in case of future repos that don't use the optimized profile. For example, in the evm utils we don't need the optimized profile specifically. Given that's a small change, I would still like to keep it.

@smol-ninja
Copy link
Member

In that case, we should close this issue and re-open it when we need other profiles. optimized profile in evm-utils is not bothering us as it only takes 1 min for the full CI to run. Though "nice to have" feature, I don't think we need it at this point.

@andreivladbrg
Copy link
Member Author

In that case, we should close this issue and re-open it when we need other profiles

why would we do that? whats the problem in keeping the issue open - or even addressing it?

@smol-ninja
Copy link
Member

smol-ninja commented Apr 8, 2025

OK lets address it then. I can see you have addressed it already. great job :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: feature New feature or request. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants