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

Always build lcli on CI #5860

Merged
merged 1 commit into from
May 29, 2024
Merged

Conversation

jimmygchen
Copy link
Member

Issue Addressed

The PR removes the "Install lcli" condition and always build lcli on CI, given it now only takes less than 3 minutes on the self-hosted runners.

So far we've had to comment out / uncomment the condition every time there's a breaking change.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels May 29, 2024
@antondlr
Copy link
Member

takes less than 3 minutes -- on (I assume) low contention runs, best we keep an eye on this during busier periods!

right now, we have an lcli binary included in the runners image which requires manual updates - how often do these breaking changes occur? if we had a weekly rebuild-trigger with manual option to fire it off, would that work?

adding 2-3 minutes to CI runs hurts me a little I guess :-) but not against it per sé.
just want to clarify that there's probably other means to get there.

@pawanjay176
Copy link
Member

how often do these breaking changes occur? if we had a weekly rebuild-trigger with manual option to fire it off, would that work?

Breaking changes are not very often imo. We would have breaking changes when adding new forks or instances like this where we change the local testnet structure.
If we could retrigger at times when we noticed a breaking change, that also is good enough for me.

@macladson
Copy link
Member

It's unfortunately not enough to just update the binary more frequently, since in the cases where we introduce a breaking change to lcli in a PR, that PR will never pass CI unless CI compiles it, since we require that exact version of lcli. I don't believe keeping it up to date with unstable is enough.

We would need some kind of logic which checks if there any changes to lcli in the PR, and in those instances compiles it from source, otherwise it's free to use the cached version.

@realbigsean
Copy link
Member

We would need some kind of logic which checks if there any changes to lcli in the PR, and in those instances compiles it from source, otherwise it's free to use the cached version.

Yes, I agree this is the way. I'm going to merge this one for now to unblock #5850

@realbigsean
Copy link
Member

@mergify queue

Copy link

mergify bot commented May 29, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 48789c7

@mergify mergify bot merged commit 48789c7 into sigp:unstable May 29, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra-ci ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants