-
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
chore: removing build dependencies and using testcontainers for container spin up #982
chore: removing build dependencies and using testcontainers for container spin up #982
Conversation
44f95c4
to
4df6755
Compare
This is cool! |
Will this clash with the CI's services? Would we need to disable those? |
it will not clash with ci services, and if we use testcontainers everywhere we should not need to have this ci services anymore - as our test will handle the containers, and more importantly provide better port mapping out of the box - see the java reference open-feature/java-sdk-contrib#860 - where we can actually skip the serlvices, and just configure the exposed port, and use the host port automatically (no port issues anymore) |
libs/providers/flagd-web/src/e2e/step-definitions/evaluation.spec.ts
Outdated
Show resolved
Hide resolved
1711005
to
a7d4749
Compare
7363611
to
3423b40
Compare
@toddbaert i kindly ask you to review this again. i changed the way we build buf files, now it is just a npx command without any fancy directory changes, and file operations (just using the cli) i removed all the setups for the e2e tests and added dedicated jest wrapper tests, which will handle the spin up and tear down of the container (those tests are a good starting point for #986) - due to that all previous tests are now exporting a function which is called via the wrapper tests. it looks big, but it is not really complicated, please review if you do have time |
0ba23a8
to
d725506
Compare
d725506
to
a2ea817
Compare
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.
Great change and great to have Testcontainers in!
21938f2
to
9e5740f
Compare
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Signed-off-by: Simon Schrottner <[email protected]>
9e5740f
to
ac010ce
Compare
@aepfli this looks excellent to me. TBH of all our gherkin suites, I was the least happy with this one, this seems like a huge improvement. Thanks a lot. I left a few nits, and I want to check one or two things locally, but I think I'll approve soon. |
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.
Amazing. This is much nicer. Can't thank you enough! Event the console feedback is much more understandable now.
I've marked this as a chore, since it's only a "feature" for developers, not for the users of the flagd/flagd-web providers. |
Co-authored-by: Lukas Reining <[email protected]> Signed-off-by: Simon Schrottner <[email protected]>
ac010ce
to
69ca1c2
Compare
This PR
Removing the need for local dependencies of buf and the need to run docker compose to run e2e tests.
The Docker container will automatically start, and buf files will be loaded via npm from buf.build as generated SDKs.
I tried to do the same thing for flagd, but the generated buf files via https://github.com/stephenh/ts-proto are not delivered as SDKs from buf.build. Hence, I can't remove the dependency on buf. If we finalize this approach, I will add the test-container execution.
to Verify run
nx e2e providers-flagd-web
locally, it should actually run without any docker spin ups