-
Notifications
You must be signed in to change notification settings - Fork 45
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
NG run-testplans
action
#111
Comments
Yup. like
sgtm! Let me know if you do end up picking this up. I'll do the same if I start on this today :) |
I was actually thinking something like:
In other words, arguments the same way The name of the files doesn't matter. I'd also consider getting rid of
Will do! |
Getting type checking is very nice. I don't know if I want to give that up. I think of "version.ts" as the source of truth for what supported versions are capable of. |
I just figured that if we pass the "extra" versions on the command-line we might as well pass all JSON files there to simplify the overall design. It seemed like a natural extension but you are right that we'd be losing type-checks on those files. Why are they declared in JSON files to begin with and not just within the array that is defined in |
They are defined in the array? https://github.com/libp2p/test-plans/blob/master/multidim-interop/versions.ts#L18 The JSON imported is simply the imageID. JSON format so that we can
|
Ah yes, I understand what is happening now, sorry. My head was already in "we are pulling docker images"-land so I forgot that we are generating these JSON files at build time and thus need to import them. For
|
Motivated by #121, here is another idea: With the new action being a composite one, I think we can get rid of the separate checkout step if we bundle (using webpack, esbuild, etc) the JavaScript up into a separate JS GitHub action. For this to work, we'd have to make another, repository local JS action (under For it to be completely stateless, we would have to migrate the Go and JS test binaries to publish containers for their test binaries to a registry, the same way we do it for the Happy to invest some time into this if we agree on the design. |
Doing so would shorten the runtime by ~10minutes as we don't have to build any more images: https://github.com/libp2p/test-plans/actions/runs/3998997158/jobs/6862340373 |
I've started to work on the composite action. |
I’m not convinced publishing to a registry is better than a large cache. I would want to investigate #113 (comment) before we require implementations to publish to a registry. |
I managed to implement the "use entire repository at the specified revision without a dedicated checkout" without this requirement so we can optimise the performance either way now! |
This can be considered done now that #123 is merged. |
Currently, this is a workflow but it could just be a composite action which would remove the artifact up and downloading.
Relevant discussion: libp2p/rust-libp2p#3331 (comment)
Suggestions on new API design for the action/test-runner: libp2p/rust-libp2p#3331 (comment)
In particular:
Happy to implement these changes once we agree on them :)
The text was updated successfully, but these errors were encountered: