Fixes to enable cross repo workflow calls#137
Conversation
jackkleeman
force-pushed
the
e2e-workflow-call
branch
4 times, most recently
from
65f8622 to
2b93c6a
Compare
jackkleeman
force-pushed
the
e2e-workflow-call
branch
from
2b93c6a to
f5aecdd
Compare
This is now working in both typescript and runtime repos via my PRs restatedev/restate#481 restatedev/sdk-typescript#86 |
jackkleeman
requested review from
slinkydeveloper and
tillrohrmann
and removed request for
slinkydeveloper
slinkydeveloper
left a comment
slinkydeveloper
left a comment
There was a problem hiding this comment.
Overall, this looks good. The only general comment I have is that we need to properly document the setup here, as I personally find it non trivial. Perhaps add some description under docs/dev/README.md (like we have in other repos)?
Another question: If I understood correctly we would not be able to trigger from the UI an e2e test with a specific restate commit and a specific sdk commit, right? Perhaps can we add it in the runtime repo? It would be very very useful to be able to do that!
We would be able to. There is a workflow call in this repo that i have set up, but I will be testing it as a follow up once this is merged as that makes it quite a bit easier. It needs me to add some new PAT secrets in this repo; hopefully that's all it needs. We will also have to consider retention as 1 day might be a bit annoying if you are testing commits from two repos |
slinkydeveloper
left a comment
slinkydeveloper
left a comment
There was a problem hiding this comment.
LGTM, just one last comment to resolve and then you can go ahead and squash/merge it.
| private var additionalContainers: MutableMap<String, GenericContainer<*>> = mutableMapOf(), | ||
| private var additionalEnv: MutableMap<String, String> = mutableMapOf(), | ||
| private var runtimeContainer: String = | ||
| System.getenv(RESTATE_RUNTIME_CONTAINER_ENV) ?: DEFAULT_RUNTIME_CONTAINER, |
There was a problem hiding this comment.
I would not do this change here, and let the fail test on an invalid config of this env. We're covered anyway by gradle.
There was a problem hiding this comment.
The problem could be that RESTATE_RUNTIME_CONTAINER_ENV is set to the empty string because of ${{ inputs.restateCommit != '' && 'localhost:5000/restatedev/restate:latest' || '' }}
There was a problem hiding this comment.
yeah - im not following francesco because the test does not currently fail if its not provided, and now it can be not provided in a new way - empty instead of null
There was a problem hiding this comment.
The problem could be that RESTATE_RUNTIME_CONTAINER_ENV is set to the empty string
No, the defaulting is covered in the gradle change in the same commit, right?
There was a problem hiding this comment.
True. I didn't realize that this change was added to the RestateDeployer as well as the build.gradle.kts. I guess with the change in build.gradle.kts it should be enough.
There was a problem hiding this comment.
@jackkleeman could you please revert this change?
There was a problem hiding this comment.
i dont understand, sorry. if there is a default set elsewhere, why do we set a default here at all?
There was a problem hiding this comment.
Because the idea behind this test utils is to separately ship them to allow users to test, so the RestateDeployer is designed in such a way that it doesn't depend on our gradle script to execute it. I have a concrete example of this in the notebook-demo.
There was a problem hiding this comment.
ok, i understand. thanks
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for creating this workflow @jackkleeman. The changes look good to me. Thanks a lot for the detailed description of the workflow which really helps understanding what is happening! +1 for merging.
3 participants
Fixes #110