Skip to content
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

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

aepfli
Copy link
Member

@aepfli aepfli commented Jul 3, 2024

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

@aepfli aepfli force-pushed the feat/testcontainers branch 2 times, most recently from 44f95c4 to 4df6755 Compare July 3, 2024 06:24
@toddbaert
Copy link
Member

This is cool!

@toddbaert
Copy link
Member

Will this clash with the CI's services? Would we need to disable those?

@aepfli
Copy link
Member Author

aepfli commented Jul 3, 2024

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)

@aepfli aepfli force-pushed the feat/testcontainers branch 4 times, most recently from 1711005 to a7d4749 Compare July 6, 2024 19:41
@aepfli aepfli requested a review from toddbaert July 6, 2024 19:42
@aepfli aepfli force-pushed the feat/testcontainers branch 6 times, most recently from 7363611 to 3423b40 Compare July 6, 2024 20:01
@aepfli
Copy link
Member Author

aepfli commented Jul 6, 2024

@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

@aepfli aepfli force-pushed the feat/testcontainers branch 4 times, most recently from 0ba23a8 to d725506 Compare July 6, 2024 20:10
@aepfli aepfli marked this pull request as ready for review July 6, 2024 20:11
@aepfli aepfli requested a review from a team as a code owner July 6, 2024 20:11
Copy link
Member

@lukas-reining lukas-reining left a 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!

libs/providers/flagd/project.json Show resolved Hide resolved
libs/providers/flagd/src/e2e/tests/rpc-provider.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@toddbaert
Copy link
Member

@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.

Copy link
Member

@toddbaert toddbaert left a 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.

@toddbaert toddbaert requested a review from beeme1mr July 9, 2024 15:06
@toddbaert toddbaert changed the title feat: removing build dependencies and using testcontainers for container spin up chore: removing build dependencies and using testcontainers for container spin up Jul 9, 2024
@toddbaert
Copy link
Member

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 <lukas.reining@codecentric.de>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli merged commit 2d64331 into open-feature:main Jul 9, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants