-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Move examples back to go-libp2p-examples #1290
Comments
I agree, having the examples here is super annoying. Probably half of the CI failures are due to people forgetting to run for x in $(find examples -type d); do pushd $x; go mod tidy; popd; done The approach taken by IPFS (ipfs/kubo#8516) doesn't work here, as we're using Unified CI and Unified CI doesn't know if a subdirectory is an example directory or not. So the plan would be:
|
For reference:
|
Regarding the diff churn and shell needed to keep modules in sync, I think 1.18's workspaces could help: golang/go#45713
It's worth noting that such a strategy is less work partially because it offers less guarantees. In particular, the current system will immediately tell you if a change in master, between two releases, breaks examples. #1052 talked briefly about whether we only care about releases or not.
Have you considered handling these example modules in a special way? For example:
It seems to me like this avoids the pain points (diff churn, manual work) without regressing on the problems we had before (maintain multiple repos, not testing examples between releases, needing to remember to re-test examples at every release). |
@marten-seemann can you explain more why the approach taken in go-ipfs won't work with unified CI? Even if unified CI is rubbing against all the modules won't that, in the worst case, just mean we do a little extra testing in CI, such as against the latest release and against the latest master? |
I dont think we really care about examples working against master; we care
about examples working with latest release.
…On Sun, Jan 9, 2022, 17:19 Adin Schmahmann ***@***.***> wrote:
@marten-seemann <https://github.com/marten-seemann> can you explain more
why the approach taken in go-ipfs won't work with unified CI? Even if
unified CI is rubbing against all the modules won't that, in the worst
case, just mean we do a little extra testing in CI, such as against the
latest release and against the latest master?
—
Reply to this email directly, view it on GitHub
<#1290 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAI4SSCJ52L26QAA3AYEFTUVGRQ5ANCNFSM5LR47HVA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
We don't want examples to show how to build with master, because we don't want people to use master.
Examples are not supposed to be tests. If examples uncover a failure that was not covered by a test, this is a good reason to add a new unit test, not to do anything different with the examples.
Without having understood how this works in detail, the current situation is unbearable till we deprecate 1.17 (in 8 months!).
CircleCI for go-ipfs is running this config: https://github.com/ipfs/go-ipfs/blob/45dac526ea32f4423b395cc55797e91353fca6c1/.circleci/main.yml#L108-L113, just in the examples directory. Unified CI wouldn't know which modules this command should and should not be run in, respectively. In general I'm all for repo consolidation, as it will speed up development. But not this one, this one is slowing us down. |
Why does this need to be done as part of the Unified CI when we could just make it a separate workflow? It seems easier to manage than another repo.
That's fine, in fact it's even easier. You can just target the module against the latest release and 🎉. For some background this was the situation in go-ipfs until it was found that the go-ipfs maintainers (perhaps me 😅) were not doing a great job updating the examples on every release so doing it incrementally was easier. So far go-ipfs hasn't gotten many complaints, although to be fair our examples are unfortunately less extensive than the go-libp2p ones.
I think the approach we're using is slowing us down, but we have at least two other approaches here that could get out of our way. One is just having a separate module in this repo and a workflow for testing it that targets the latest released version. The other is to have a separate module and workflow that tests against the same dependencies present in the latest branch as we currently do in go-ipfs.
Maybe, and technically you may be right but realistically people's first stop is probably going to be the go-libp2p repo, even the name of the proposed repo is Personally I've experienced that relying on checklists when I could've used automation tends to be a burden that results in mistakes and frustration. However, @marten-seemann and @vyzo are the maintainers doing most of the work on go-libp2p these days so if they are ok with the burden of maintaining an extra repo and updating it soon after go-libp2p releases are tagged then it seems like their call to make. If doing the checklist manually results in the examples lagging behind again I'm sure they'll hear about it in some friendly issues 😄. |
For now we have removed the replace directives, and added to the release checklist, which resolves our immediate pain point while we are working on v0.18. |
Thank you for the lively discussion! We've removed the replace directives (as @mvdan suggested), which means that we don't have to update the examples for every commit. It also means that the examples are now more useful (you can just copy-paste them, including go.mod). |
Examples were moved in repo with good intentions, namely keeping them up-to-date.
However this has had very deleterious effects: it requires all prs that pull some dep (pretty much all of them do) to contitnuously babysit examples, and this affects development velocity and developer morale; let alone the diff bloat in every pr.
We propose to move them back where they belong, to their own repo, and add to the release checklist a step that ensures they are kept up to date.
Have cake, eat it too.
The text was updated successfully, but these errors were encountered: