-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/redpanda/v1 rc #16
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
base: main
Are you sure you want to change the base?
Conversation
I didn't realize me commenting would cause @copilot to do work. I'll leave it up to you @redpanda-f if/when it makes sense for copilot to be engaged. Feel free to close #18 if it isn't helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how large this PR is, this seems like something that could be separated out.
Also, does this actually work on github runners? It seems like there is hackery going on here to make it work. Should we just use self-hosted runners? We can get use a similar setup to what we have with Lotus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no hack here, there is a whole bunch of caching between steps that's needed to make it all run in reasonable time.
The large-ness of this PR is okay since this is still an active development non-stable repo. Breaking this apart is more effort than necessary.
In my opinion, once we have a stable V1, having smaller PRs and CI validation makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think I was maybe misunderstanding what this file is. This is about smoke testing the foc-localnet machineri, not actually running an e2e scenario like "basic deal + retrieaval" that is discussed in #8 is that right? Is this ci.yml is intended to catch problems with foc-localnet code/configuration itself? Basically, if there is an issue with how we create docker images or start containers, this is intended to catch it right? (In my updated updated understanding, this ci.yml is serving as an integration test of all the code in this PR, but it's not serving as a way to validate Synapse for example.). Let me know if I'm misunderstanding things...
Regardless, it would be good to comment in this file about its purpose so there isn't confusion in the future.
(And if this file is really about validating the foc-localnet code itself then I agree it belongs in this PR. Apologies about comments I made elsewhere that suggested this file doesn't belong in PR. In my mind, #8 doesn't belong in this PR. Testing to validate the work of #5 belongs in this PR...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about smoke testing the foc-localnet machineri, not actually running an e2e scenario like "basic deal + retrieaval" that is discussed in #8 is that right?
start undertakes a basic check of deal and retrieval before it classifies a start as successful. So, that simple test is there.
|
@redpanda-f : given the PR needs a round of you to go over it yourself (and potentially have AI to review as well for duplication/simplification), I have marked this as draft to signal to reviewers that it isn't blocked on their review yet. Once you're ready for review, please set it "ready for review" and change the status on the project board to be "awaiting review". Please also feel free to request/re-request reviewers in GitHub. In total, that should make it clear that this is ready to the rest of the working group that you're needing review. |
No description provided.